feat(sharing): introduce friend settings schema and refine share request logic #73
Conversation
- Introduce new friendPlayer and friendSetting tables to manage relationships between users and their friends, including permissions and settings. - Update relations in index.ts to include new relationships for friendPlayers and friendSettings, enhancing data structure and retrieval capabilities.
- Change default values for allowSharedGames, allowSharedPlayers, allowSharedLocation, and allowSharedMatches to true in the friendSetting schema, enhancing user permissions and settings management.
…saction management - Refactor share request logic to utilize database transactions for better consistency and error handling. - Update share request creation process to handle multiple item types (games, matches, locations, players, scoresheet) more efficiently. - Enhance error handling and response messages for improved user feedback during sharing operations.
… settings - Implement status assignment for share requests, setting it to "accepted" or "pending" based on user friend settings for players and games. - Enhance share request creation logic to improve user experience and streamline sharing operations.
…friend validation - Introduce handling for shared locations in share requests, ensuring proper validation of friends before sharing. - Update transaction management to improve error handling and streamline the sharing process for games, matches, and locations. - Refactor existing logic to incorporate new shared location schema and enhance user feedback during sharing operations.
- Replace instances of the transaction variable to ensure consistent usage of `tx2` for database queries and inserts in share request handling. - Improve code clarity and maintainability by standardizing transaction management across various share request operations.
…anagement - Consolidate transaction handling in share request logic to improve consistency and maintainability. - Streamline the process for sharing players, matches, and locations, ensuring proper validation and error handling. - Enhance user feedback by refining response messages during sharing operations.
…ations, and players - Enhance match creation process to include sharing functionality with friends based on their settings. - Introduce detailed transaction management for sharing matches, locations, and players, ensuring proper validation and error handling. - Streamline the sharing process by incorporating new schemas for shared matches, locations, and players, improving user experience and feedback during sharing operations.
…n handling - Update sharing functionality to utilize friend settings more effectively, allowing for granular control over shared matches, locations, and players. - Introduce new relationships in the database schema to support friend connections and their associated settings. - Improve transaction management for share requests, ensuring accurate permission assignment and status handling based on friend configurations.
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThis update introduces a comprehensive friend management and sharing system across the application. It adds friend-specific sharing settings, enables granular control over shared matches, games, players, and locations, and integrates Clerk user data for enriched friend profiles. The database schema and API logic are refactored to use a boolean Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant API
participant DB
participant Clerk
participant Friend
User->>API: Create Match
API->>DB: Insert match, players, rounds
API->>DB: Fetch match details & friends
loop For each eligible Friend
API->>DB: Check friend sharing settings
alt Auto-sharing enabled
API->>DB: Create share request (auto-accept if allowed)
API->>DB: Share match, location, game, players as needed
else
API->>DB: Create share request (pending)
end
end
API->>User: Return created match (shared as needed)
sequenceDiagram
participant User
participant API
participant DB
participant Clerk
User->>API: View Friend Profile
API->>DB: Fetch friend, linked player, shared entities, settings
API->>Clerk: Fetch Clerk user data for friend
API->>User: Return profile, stats, shared items, settings
Poem
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure ✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
packages/api/src/routers/sharing/share-request.ts (1)
665-673: 💡 Verification agent🧩 Analysis chain
Writes executed outside the enclosing transaction break atomicity
Inside the outer
ctx.db.transaction, you are callingctx.db.insert(...)instead oftx.insert(...).
If this insert fails or any later statement in the outer transaction is rolled-back, the location share created here will remain committed, leaving the DB in an inconsistent state.- await ctx.db.insert(shareRequest).values({ + await tx.insert(shareRequest).values({Search for any other remaining
ctx.db.*calls in this file to ensure every query inside the transaction uses the sametxhandle.
🏁 Script executed:
#!/bin/bash # Verify there are no stray ctx.db usages in share-request router rg --line-number --context 2 $'ctx\\.db\\.' packages/api/src/routers/sharing/share-request.tsLength of output: 1370
Ensure atomicity by using
tx.insertinside the transactionInside the mutation starting at line 628 of
packages/api/src/routers/sharing/share-request.ts, the insert on line 666 usesctx.db.insert, which executes outside the enclosingtxand breaks the transaction’s atomicity. Update it as follows:• File:
packages/api/src/routers/sharing/share-request.ts
Location: within the callback ofctx.db.transaction(async (tx) => { … }), around line 666- await ctx.db.insert(shareRequest).values({ + await tx.insert(shareRequest).values({
🧹 Nitpick comments (2)
packages/api/src/routers/match.ts (1)
415-619: Consider extracting the friend sharing logic to a separate function for maintainability.While the implementation is solid, the friend sharing logic (lines 415-619) is quite complex and lengthy. Consider extracting it to a separate helper function to improve readability and maintainability of the
createMatchmutation.+ // Add this helper function before the matchRouter definition + async function shareMatchWithFriends( + tx: any, + ctx: any, + createdMatch: any, + returningMatch: any, + shareFriends: any[] + ) { + for (const shareFriend of shareFriends) { + await tx.transaction(async (innerTx) => { + // Move the entire sharing logic here + // Replace tx with innerTx + // ...existing code from lines 416-618... + }); + } + } // Then in the createMatch mutation, replace lines 415-619 with: + await shareMatchWithFriends( + transaction, + ctx, + createdMatch, + returningMatch, + shareFriends + );packages/api/src/routers/sharing/share-request.ts (1)
68-104: Large, deeply-nested loops duplicate sharing logic – consider extracting helpersThe
for … ofloops for matches, locations, players and scoresheets are almost identical in the Game, Match and Player mutations. Moving this logic into reusable helper functions (e.g.createLocationShare(tx, …)orautoAcceptIfNeeded(tx, settings, …)) will:
• reduce ~1 000 lines of repetition,
• make the transaction flow easier to audit,
• improve testability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/api/src/routers/match.ts(3 hunks)packages/api/src/routers/sharing/share-request.ts(7 hunks)packages/db/src/relations/index.ts(4 hunks)packages/db/src/schema/friendPlayer.ts(1 hunks)packages/db/src/schema/friendSetting.ts(1 hunks)packages/db/src/schema/index.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
packages/api/src/routers/match.ts (1)
packages/db/src/zodSchema/index.ts (2)
selectSharedLocationSchema(105-105)selectSharedPlayerSchema(75-75)
packages/api/src/routers/sharing/share-request.ts (1)
packages/db/src/zodSchema/index.ts (2)
selectSharedLocationSchema(105-105)selectSharedPlayerSchema(75-75)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: lint
🔇 Additional comments (12)
packages/db/src/schema/index.ts (1)
24-25: Well-structured schema exports for the new friend sharing system.These export declarations appropriately expose the new
friendPlayerandfriendSettingschemas, which form the foundation of the per-friend sharing preferences system described in the PR objectives.packages/db/src/schema/friendPlayer.ts (2)
17-50: Great schema design with appropriate constraints and permissions model.The
friend_playertable schema effectively links friends to players with permission controls. The use of a permission enum with "view"/"edit" options provides the granular control described in the PR objectives, while the default of "view" implements principle of least privilege.
40-48: Thorough index implementation for query optimization.The indexes on
createdById,friendId, andplayerIdwill improve query performance for the relationship-heavy queries seen in the match router. The unique constraint onfriendIdandplayerIdcorrectly prevents duplicate relationships.packages/db/src/schema/friendSetting.ts (2)
16-63: Comprehensive friend settings schema with secure defaults.The schema implements an extensive set of permission and sharing controls while maintaining secure defaults. Auto-sharing features default to
falserequiring explicit opt-in, while the ability to receive shares defaults totruefor good user experience.
70-78: Well-implemented indexes and constraints.The indexes will optimize the queries seen in the match router that filter based on these columns. The unique constraint correctly ensures only one settings record exists per creator-friend relationship.
packages/api/src/routers/match.ts (7)
327-374: Thorough match query with deep relational data.The query retrieves all necessary data for sharing decisions in a single operation, including nested friend relationships and settings. This approach is efficient and avoids multiple queries.
375-414: Effective friend filtering based on sharing permissions.The code elegantly filters friends eligible for sharing by evaluating both the friend's settings (
autoShareMatches) and the user's own settings (allowSharedMatches), ensuring mutual consent for sharing.
415-437: Well-implemented transactional share request creation.Using nested transactions ensures atomicity when creating share requests. The status is correctly set based on
autoAcceptMatchespreference, automatically differentiating between auto-accepted and pending shares.
438-484: Thorough location sharing with existence checks.The location sharing logic properly checks for existing shared locations before creating new ones, preventing duplicates. The permission settings are correctly propagated from friend settings.
485-526: Efficient game sharing with reuse of existing shares.The code first checks for existing shared games before creating new ones, preventing duplicate shares and efficiently reusing existing relationships.
527-549: Conditional shared match creation with proper references.The shared match creation correctly references the shared game and location (if available), maintaining referential integrity in the sharing graph.
550-617: Comprehensive player sharing with permission inheritance.The player sharing implementation handles both direct player sharing and match player association, correctly linking shared players to shared matches when appropriate. The permission model is consistently applied across all entity types.
…riendPlayer schema - Replace transaction variable in share request logic for consistency. - Modify friendSettings relation to support multiple friend settings. - Enhance unique constraints in friendPlayer schema to improve data integrity and relationships.
…improved maintainability - Refactor sharing functionality into a new `shareMatchWithFriends` function to streamline the process of sharing matches, locations, and players with friends. - Enhance transaction management within the new function, ensuring consistent handling of share requests and permissions based on friend settings. - Improve code clarity and maintainability by reducing complexity in the main match router logic.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
packages/api/src/routers/match.ts (4)
328-418: Strong match data fetching for sharing, but consider query optimization.The code effectively fetches the newly created match with all necessary related data for sharing, but the deeply nested query might cause performance issues with large datasets.
Consider optimizing this query by:
- Fetching only essential data needed for sharing decisions
- Using separate, targeted queries for specific relationships if needed
- Adding pagination if dealing with potentially large numbers of players or friends
- const createdMatch = await transaction.query.match.findFirst({ - where: { - id: returningMatch.id, - }, - with: { - scoresheet: true, - game: true, - matchPlayers: { - with: { - player: { - with: { - friendPlayer: { - where: { - createdById: ctx.userId, - }, - with: { - friend: { - with: { - friend: { - with: { - friends: { - where: { - friendId: ctx.userId, - }, - with: { - friendSetting: true, - }, - }, - }, - }, - friendSetting: true, - }, - }, - }, - }, - }, - }, - }, - }, - location: true, - }, - }); + // First, fetch just the match with basic relationships + const createdMatch = await transaction.query.match.findFirst({ + where: { id: returningMatch.id }, + with: { + scoresheet: true, + game: true, + location: true, + matchPlayers: { + with: { player: { columns: { id: true } } } + } + } + }); + + // Then, fetch friend settings in a separate query if needed + const playerIds = createdMatch.matchPlayers.map(mp => mp.player.id); + const friendSettings = await transaction.query.friendPlayer.findMany({ + where: { + playerId: { in: playerIds }, + createdById: ctx.userId + }, + with: { + friend: { + with: { + friendSetting: true, + friend: { + with: { + friends: { + where: { friendId: ctx.userId }, + with: { friendSetting: true } + } + } + } + } + } + } + }); + + // Then map friend settings to players as needed
376-416: Clean but complex filtering logic for shareable friends.The logic to determine which friends should receive shared content is functional but complex, making maintenance challenging. Consider refactoring this block into a dedicated helper function.
- const shareFriends = createdMatch.matchPlayers - .flatMap((matchPlayer) => { - const returnedFriend = matchPlayer.player.friendPlayer?.friend; - const returnedFriendSetting = returnedFriend?.friend.friends.find( - (friend) => friend.friendId === ctx.userId, - )?.friendSetting; - if ( - returnedFriend?.friendSetting?.autoShareMatches === true && - returnedFriendSetting?.allowSharedMatches === true - ) { - return { - friendUserId: returnedFriend.friendId, - shareLocation: - returnedFriend.friendSetting.includeLocationWithMatch === - true, - sharePlayers: - returnedFriend.friendSetting.sharePlayersWithMatch === true, - defaultPermissionForMatches: - returnedFriend.friendSetting.defaultPermissionForMatches, - defaultPermissionForPlayers: - returnedFriend.friendSetting.defaultPermissionForPlayers, - defaultPermissionForLocation: - returnedFriend.friendSetting.defaultPermissionForLocation, - defaultPermissionForGame: - returnedFriend.friendSetting.defaultPermissionForGame, - allowSharedPlayers: - returnedFriendSetting.allowSharedPlayers === true, - allowSharedLocation: - returnedFriendSetting.allowSharedLocation === true, - autoAcceptMatches: - returnedFriendSetting.autoAcceptMatches === true, - autoAcceptPlayers: - returnedFriendSetting.autoAcceptPlayers === true, - autoAcceptLocation: - returnedFriendSetting.autoAcceptLocation === true, - }; - } - return false; - }) - .filter((friend) => friend !== false); + // Extract to a descriptive helper function + const shareFriends = getShareableFriendsFromMatch(createdMatch, ctx.userId);With a helper function definition:
function getShareableFriendsFromMatch(match, userId) { return match.matchPlayers .flatMap((matchPlayer) => { const returnedFriend = matchPlayer.player.friendPlayer?.friend; const returnedFriendSetting = returnedFriend?.friend.friends.find( (friend) => friend.friendId === userId, )?.friendSetting; // Early exit if no auto-sharing or matches not allowed if ( !returnedFriend?.friendSetting?.autoShareMatches || !returnedFriendSetting?.allowSharedMatches ) { return false; } // Return friend with all required settings return { friendUserId: returnedFriend.friendId, shareLocation: Boolean(returnedFriend.friendSetting.includeLocationWithMatch), sharePlayers: Boolean(returnedFriend.friendSetting.sharePlayersWithMatch), defaultPermissionForMatches: returnedFriend.friendSetting.defaultPermissionForMatches, defaultPermissionForPlayers: returnedFriend.friendSetting.defaultPermissionForPlayers, defaultPermissionForLocation: returnedFriend.friendSetting.defaultPermissionForLocation, defaultPermissionForGame: returnedFriend.friendSetting.defaultPermissionForGame, allowSharedPlayers: Boolean(returnedFriendSetting.allowSharedPlayers), allowSharedLocation: Boolean(returnedFriendSetting.allowSharedLocation), autoAcceptMatches: Boolean(returnedFriendSetting.autoAcceptMatches), autoAcceptPlayers: Boolean(returnedFriendSetting.autoAcceptPlayers), autoAcceptLocation: Boolean(returnedFriendSetting.autoAcceptLocation), }; }) .filter((friend) => friend !== false); }
1507-1510: Complex type definition could be simplified.The
TransactionTypedefinition uses nested type parameter extraction, which is difficult to read and understand at a glance.- export type TransactionType = Parameters< - Parameters<DatabaseType["transaction"]>[0] - >[0]; + // More readable approach using intermediary types + type TransactionCallback = Parameters<DatabaseType["transaction"]>[0]; + export type TransactionType = Parameters<TransactionCallback>[0];
1614-1654: Reused logic for finding/creating shared game could be extracted.This pattern of checking for an existing entity and creating it if not found is repeated multiple times throughout the code. Consider extracting it to a reusable helper function.
- const existingSharedGame = await tx.query.sharedGame.findFirst({ - where: { - gameId: createdMatch.game.id, - sharedWithId: shareFriend.friendUserId, - ownerId: userId, - }, - }); - if (!existingSharedGame) { - const [createdSharedGame] = await tx - .insert(sharedGame) - .values({ - ownerId: userId, - sharedWithId: shareFriend.friendUserId, - gameId: createdMatch.game.id, - permission: shareFriend.defaultPermissionForGame, - }) - .returning(); - if (!createdSharedGame) { - throw new TRPCError({ - code: "INTERNAL_SERVER_ERROR", - message: "Failed to generate share.", - }); - } - returnedSharedGame = createdSharedGame; - } else { - returnedSharedGame = existingSharedGame; - } + // Using a generic helper function + returnedSharedGame = await findOrCreateSharedEntity( + tx, + sharedGame, + { + gameId: createdMatch.game.id, + sharedWithId: shareFriend.friendUserId, + ownerId: userId, + }, + { + ownerId: userId, + sharedWithId: shareFriend.friendUserId, + gameId: createdMatch.game.id, + permission: shareFriend.defaultPermissionForGame, + }, + "Failed to generate share." + );With a helper function like:
async function findOrCreateSharedEntity<T extends Record<string, any>>( tx: TransactionType, table: any, whereClause: Record<string, any>, insertValues: Record<string, any>, errorMessage: string ): Promise<T> { const existing = await tx.query[table.name].findFirst({ where: whereClause, }); if (existing) return existing; const [created] = await tx .insert(table) .values(insertValues) .returning(); if (!created) { throw new TRPCError({ code: "INTERNAL_SERVER_ERROR", message: errorMessage, }); } return created; }packages/api/src/routers/sharing/share-request.ts (3)
192-211: Good user preference validation but duplicated across mutations.The user preference validation logic is correctly implemented but is duplicated across the three sharing mutations (
requestShareGame,requestShareMatch,requestSharePlayer). This could be extracted to a reusable function.- const recipientSettings = - await tx2.query.userSharingPreference.findFirst({ - where: { - userId: friendToShareTo.id, - }, - }); - - if (recipientSettings?.allowSharing === "none") { - shareMessages.push({ - success: false, - message: `User ${recipientSettings.userId} does not allow sharing.`, - }); - return false; - } - const returnedFriend = await tx2.query.friend.findFirst({ - where: { - friendId: ctx.userId, - userId: friendToShareTo.id, - }, - }); - if (!returnedFriend) { - shareMessages.push({ - success: false, - message: `User ${friendToShareTo.id} does not exist.`, - }); - return false; - } + // Extract to a helper function + const validationResult = await validateFriendSharingPermissions( + tx2, + ctx.userId, + friendToShareTo.id + ); + + if (!validationResult.success) { + shareMessages.push({ + success: false, + message: validationResult.message, + }); + return false; + } + + const returnedFriend = validationResult.friend;With a helper function like:
async function validateFriendSharingPermissions( tx: TransactionType, currentUserId: number, friendId: number ) { // Check sharing preferences const recipientSettings = await tx.query.userSharingPreference.findFirst({ where: { userId: friendId }, }); if (recipientSettings?.allowSharing === "none") { return { success: false, message: `User ${recipientSettings.userId} does not allow sharing.`, }; } // Check friend relationship exists const returnedFriend = await tx.query.friend.findFirst({ where: { friendId: currentUserId, userId: friendId, }, }); if (!returnedFriend) { return { success: false, message: `User ${friendId} does not exist.`, }; } return { success: true, friend: returnedFriend, }; }
383-407: Well-implemented location sharing logic but repeats a pattern.The location sharing logic correctly handles permission checks, share request creation, and entity creation for auto-acceptance. However, this pattern is repeated in multiple places in the code.
This functionality could be extracted to a reusable function to reduce duplication:
/** * Handles the sharing of a location with a friend * @returns The created shared location if auto-accepted, or null */ async function handleLocationSharing( tx: TransactionType, ownerId: number, locationId: number, friendId: number, parentShareId: number, friendSettings: any, permission: string, expiresAt: Date | null ) { // Check for existing share request const existingShareLocation = await tx.query.shareRequest.findFirst({ where: { OR: [ { itemId: locationId, itemType: "location", ownerId: ownerId, sharedWithId: friendId, parentShareId: parentShareId, }, { itemId: locationId, itemType: "location", ownerId: ownerId, sharedWithId: friendId, status: "accepted", }, ], }, }); // Skip if already shared if (!existingShareLocation) { // Create share request await tx.insert(shareRequest).values({ ownerId: ownerId, sharedWithId: friendId, itemType: "location", itemId: locationId, permission: permission, status: friendSettings?.autoAcceptLocation ? "accepted" : "pending", parentShareId: parentShareId, expiresAt: expiresAt, }); // Handle auto-acceptance if enabled if (friendSettings?.autoAcceptLocation) { const existingSharedLocation = await tx.query.sharedLocation.findFirst({ where: { locationId: locationId, sharedWithId: friendId, ownerId: ownerId, }, }); if (!existingSharedLocation) { const [createdSharedLocation] = await tx .insert(sharedLocation) .values({ ownerId: ownerId, sharedWithId: friendId, locationId: locationId, permission: friendSettings.defaultPermissionForLocation, }) .returning(); if (!createdSharedLocation) { throw new TRPCError({ code: "INTERNAL_SERVER_ERROR", message: "Failed to generate location share.", }); } return createdSharedLocation; } else { return existingSharedLocation; } } } return null; }This function could then be used in all places where location sharing needs to be handled.
1783-1957: Block creates deeply nested conditionals that reduce readability.The player sharing logic within match sharing contains deeply nested conditionals that reduce code readability and make maintenance difficult.
Consider refactoring with early returns or by extracting logic into separate functions:
- if (!createdSharedScoresheet) { - throw new TRPCError({ - code: "INTERNAL_SERVER_ERROR", - message: "Failed to generate share.", - }); - } - } - } - } - let returnedSharedMatch: z.infer< - typeof selectSharedMatchSchema - > | null = null; - if (friendSettings?.autoAcceptMatches) { - const existingSharedMatch = - await tx2.query.sharedMatch.findFirst({ - where: { - matchId: matchToShare.matchId, - sharedWithId: friendToShareTo.id, - ownerId: ctx.userId, - }, - }); - if (!existingSharedMatch) { - if (!returnedSharedGame) { - const existingSharedGame = - await tx2.query.sharedGame.findFirst({ - where: { - gameId: returnedMatch.gameId, - sharedWithId: friendToShareTo.id, - ownerId: ctx.userId, - }, - }); - if (!existingSharedGame) { - throw new TRPCError({ - code: "INTERNAL_SERVER_ERROR", - message: "Failed to generate share.", - }); - } - returnedSharedGame = existingSharedGame; - } - const [createdSharedMatch] = await tx2 - .insert(sharedMatch) - .values({ - ownerId: ctx.userId, - sharedWithId: friendToShareTo.id, - sharedGameId: returnedSharedGame.id, - matchId: matchToShare.matchId, - sharedLocationId: - returnedSharedLocation?.id ?? undefined, - permission: friendSettings.defaultPermissionForMatches, - }) - .returning(); - if (!createdSharedMatch) { - throw new TRPCError({ - code: "INTERNAL_SERVER_ERROR", - message: "Failed to generate share.", - }); - } - returnedSharedMatch = createdSharedMatch; - } else { - returnedSharedMatch = existingSharedMatch; - } - } + // Extract to a dedicated function that handles all the match sharing logic + let returnedSharedMatch = await handleMatchSharing( + tx2, + ctx.userId, + friendToShareTo.id, + matchToShare.matchId, + returnedMatch.gameId, + returnedSharedGame, + returnedSharedLocation, + friendSettings + );With helper functions like:
async function handleMatchSharing( tx: TransactionType, ownerId: number, friendId: number, matchId: number, gameId: number, sharedGame: any | null, sharedLocation: any | null, friendSettings: any ) { // Skip if auto-accept not enabled if (!friendSettings?.autoAcceptMatches) { return null; } // Check for existing shared match const existingSharedMatch = await tx.query.sharedMatch.findFirst({ where: { matchId: matchId, sharedWithId: friendId, ownerId: ownerId, }, }); if (existingSharedMatch) { return existingSharedMatch; } // Ensure we have a shared game reference let sharedGameRef = sharedGame; if (!sharedGameRef) { sharedGameRef = await tx.query.sharedGame.findFirst({ where: { gameId: gameId, sharedWithId: friendId, ownerId: ownerId, }, }); if (!sharedGameRef) { throw new TRPCError({ code: "INTERNAL_SERVER_ERROR", message: "Failed to generate share: shared game not found.", }); } } // Create the shared match const [createdSharedMatch] = await tx .insert(sharedMatch) .values({ ownerId: ownerId, sharedWithId: friendId, sharedGameId: sharedGameRef.id, matchId: matchId, sharedLocationId: sharedLocation?.id, permission: friendSettings.defaultPermissionForMatches, }) .returning(); if (!createdSharedMatch) { throw new TRPCError({ code: "INTERNAL_SERVER_ERROR", message: "Failed to generate share.", }); } return createdSharedMatch; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/api/src/routers/match.ts(4 hunks)packages/api/src/routers/sharing/share-request.ts(8 hunks)packages/db/src/relations/index.ts(4 hunks)packages/db/src/schema/friendPlayer.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/db/src/schema/friendPlayer.ts
- packages/db/src/relations/index.ts
🧰 Additional context used
🧬 Code Graph Analysis (2)
packages/api/src/routers/match.ts (2)
packages/db/src/client.ts (1)
db(23-26)packages/db/src/zodSchema/index.ts (2)
selectSharedLocationSchema(105-105)selectSharedPlayerSchema(75-75)
packages/api/src/routers/sharing/share-request.ts (1)
packages/db/src/zodSchema/index.ts (2)
selectSharedLocationSchema(105-105)selectSharedPlayerSchema(75-75)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: lint
🔇 Additional comments (5)
packages/api/src/routers/match.ts (2)
1511-1544: Well-structured function signature with comprehensive type definitions.The
shareMatchWithFriendsfunction has a clear signature with properly typed parameters that document the expected input data structure.
1545-1546: Good use of nested transactions for isolation.Using a nested transaction for each friend's sharing operation is a solid approach that provides isolation between different friend shares.
packages/api/src/routers/sharing/share-request.ts (3)
68-178: Great improvement using transactions for atomicity.The refactoring of the
requestShareGamemutation to use a database transaction is a significant improvement. It ensures that all database operations either succeed together or fail together, preventing partial completion issues.
1246-1261: Good transaction usage inrequestSharePlayer.This function has been properly refactored to use a database transaction, ensuring all operations are atomic.
1400-1418: Improved return structure includes detailed success messages.The updated return values from share operations include detailed success/failure messages per friend, which is a great improvement for client-side feedback.
…maintainability - Refactor sharing functionality into dedicated helper functions for handling location and match sharing, enhancing code organization and readability. - Streamline transaction management across share requests, ensuring consistent handling of permissions and statuses based on friend settings. - Improve validation and error handling during sharing operations, providing clearer feedback for users and reducing complexity in the main router logic.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
packages/api/src/routers/match.ts (3)
1508-1609: Complex sharing logic would benefit from smaller functions.While functionally correct, the
shareMatchWithFriendsfunction coordinates multiple operations (share requests, location sharing, game sharing, match creation, player sharing) within nested transactions. Breaking this down into smaller, more focused functions would improve testability and maintainability.Consider restructuring to further divide responsibilities:
async function shareMatchWithFriends( transaction: TransactionType, userId: number, createdMatch: {...}, shareFriends: {...}[], ) { for (const shareFriend of shareFriends) { await transaction.transaction(async (tx) => { - let returnedSharedLocation: z.infer<typeof selectSharedLocationSchema> | null = null; - const [newShare] = await tx - .insert(shareRequest) - .values({ - ownerId: userId, - sharedWithId: shareFriend.friendUserId, - itemType: "match", - itemId: createdMatch.id, - status: shareFriend.autoAcceptMatches ? "accepted" : "pending", - permission: shareFriend.defaultPermissionForMatches, - expiresAt: null, - }) - .returning(); - if (!newShare) { - throw new TRPCError({...}); - } + // Create primary share request for the match + const newShare = await createShareRequest(tx, { + ownerId: userId, + sharedWithId: shareFriend.friendUserId, + itemType: "match", + itemId: createdMatch.id, + status: shareFriend.autoAcceptMatches ? "accepted" : "pending", + permission: shareFriend.defaultPermissionForMatches, + }); // Rest of the function with similar modularization }); } } // Helper that encapsulates creating a share request with error handling async function createShareRequest(tx: TransactionType, shareData: {...}) { const [newShare] = await tx.insert(shareRequest).values({...}).returning(); if (!newShare) { throw new TRPCError({ code: "INTERNAL_SERVER_ERROR", message: "Failed to generate share.", }); } return newShare; }
1610-1672: Add JSDoc comments to document function parameters.The
handleLocationSharingfunction is well-structured but lacks documentation about its parameters and return value. Adding JSDoc would make its usage clearer, especially given the complexity of the friend setting object.Add JSDoc documentation:
/** * Handles sharing a location with a friend, creating necessary requests and shared entities * * @param transaction - The database transaction * @param ownerId - ID of the user sharing the location * @param locationId - ID of the location being shared * @param shareFriend - Object containing friend's sharing preferences * @param newShareId - ID of the parent share request * @returns The created shared location if auto-accepted, or null */ async function handleLocationSharing(...)
1774-1801: Separate concerns in the player sharing logic.The
handlePlayerSharingfunction combines multiple responsibilities: creating share requests, managing shared player records, and creating shared match player records. This makes the function harder to test and maintain.Consider splitting this into separate functions:
async function handlePlayerSharing(...) { // Only handle the player sharing part const returnedSharedPlayer = await createOrFindSharedPlayer(...); // Delegate match player creation to another function if (returnedSharedMatch) { await createSharedMatchPlayer(...); } } async function createOrFindSharedPlayer(...) { // Logic to create share request and manage shared player record } async function createSharedMatchPlayer(...) { // Logic to create shared match player record }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/api/src/routers/match.ts(4 hunks)packages/api/src/routers/sharing/share-request.ts(9 hunks)packages/db/src/client.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/db/src/client.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/api/src/routers/match.ts (2)
packages/db/src/client.ts (1)
TransactionType(29-29)packages/db/src/zodSchema/index.ts (2)
selectSharedLocationSchema(105-105)selectSharedPlayerSchema(75-75)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: lint
🔇 Additional comments (9)
packages/api/src/routers/match.ts (3)
327-343: Good addition of proper error handling for failed match retrieval.The code correctly fetches the created match with all necessary relations and throws an appropriate error if the match isn't found, ensuring robustness in the auto-sharing feature.
350-415: Comprehensive implementation of friend-aware match sharing.The code successfully identifies friend relationships among match players and filters them based on sharing preferences. The implementation:
- Efficiently queries
friendPlayerrecords with all necessary relations- Applies appropriate filtering based on both sender and recipient sharing preferences
- Constructs a detailed permission object for each eligible friend
This approach ensures that sharing respects both users' preferences before attempting any database operations.
417-422: Well-structured approach to share propagation.Delegating the actual sharing work to a helper function keeps the main mutation clean and focused on match creation, while passing all necessary context for the sharing operation.
packages/api/src/routers/sharing/share-request.ts (6)
69-79: Improved transaction handling for database operations.The code now consistently uses transactions for all database operations in the sharing process, which ensures atomicity and prevents partial mutations in case of errors.
185-205: Good refactoring of friend sharing logic.Moving the complex friend sharing logic to a separate helper function (
requestShareGameToFriend) improves readability and maintainability of the main mutation. The approach of collecting and returning detailed success/failure messages provides good UX feedback.
365-419: Well-structured permission validation before sharing.The code properly validates friendship existence and sharing permissions through the reusable
validateFriendSharingPermissionshelper before proceeding with sharing operations. This ensures robust permission checks and clear error messaging.
1902-1939: Good extraction of permission validation logic.The
validateFriendSharingPermissionsfunction is well-designed, focused on a single responsibility, and provides clear success/failure results with descriptive messages. This improves code reuse and simplifies the calling functions.
1944-2047: Well-documented location sharing helper.The
handleLocationSharingfunction includes good documentation about its purpose and return value. The function is well-structured with clear logic for checking existing shares, creating share requests, and handling auto-acceptance.
2048-2155: Thorough error handling in match sharing.The
handleMatchSharingfunction includes comprehensive error handling, particularly when dealing with the shared game dependency. It correctly throws descriptive TRPCErrors when critical operations fail, which will help with debugging and client error messaging.
… permission handling - Introduce a new function `handleLocationSharing` to manage the sharing of locations with friends, allowing for auto-acceptance and permission settings. - Enhance transaction management for share requests related to locations, ensuring proper validation and error handling. - Streamline the process of checking existing share requests and shared locations, improving overall clarity and maintainability of the sharing logic.
…g for matches and locations - Introduce a new `sharedMatchWithFriends` function to streamline the sharing process for matches, including location and player sharing based on friend settings. - Refactor `handleLocationSharing` to improve clarity and maintainability, ensuring proper permission handling and transaction management. - Update transaction logic to enhance error handling and validation during share requests, improving overall user experience and feedback.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/api/src/routers/match.ts (1)
1614-1674: 🛠️ Refactor suggestionAdd return type annotation to handleGameSharing
The function has an inconsistent return pattern (returns a shared game object in some cases and null in others) without a proper type annotation. This could lead to runtime errors if consumer code assumes a consistent return type.
Add a proper return type annotation:
- async function handleGameSharing( + async function handleGameSharing( transaction: TransactionType, ownerId: number, gameId: number, shareFriend: {...}, newShareId: number, - ) { + ): Promise<z.infer<typeof selectSharedGameSchema> | null> { // ...function implementation... }
🧹 Nitpick comments (9)
packages/api/src/utils/sharing.ts (1)
93-117: Consider consolidating duplicate location query parameters.There's a duplicate query pattern between lines 64-66 and 94-95 for finding a shared location.
- const existingShared = await transaction.query.sharedLocation.findFirst({ - where: { locationId, sharedWithId: friendId, ownerId }, - }); + // Create a reusable function for getting shared location + async function getSharedLocation(tx: TransactionType, locId: number, friendId: number, ownerId: number) { + return tx.query.sharedLocation.findFirst({ + where: { locationId: locId, sharedWithId: friendId, ownerId }, + }); + } + + const existingShared = await getSharedLocation(transaction, locationId, friendId, ownerId);packages/api/src/routers/match.ts (4)
351-374: Complex query to determine friend eligibility for sharing.The query to find friend players includes multiple nested relations to access both the friend's settings and their reciprocal settings toward the current user. This is necessary for proper permission validation but introduces complexity.
Consider extracting this query to a separate function to improve readability:
async function getFriendPlayersForSharing(db: any, userId: number, playerIds: number[]) { return db.query.friendPlayer.findMany({ where: { createdById: userId, friendId: { in: playerIds, }, }, with: { friend: { with: { friendSetting: true, friend: { with: { friends: { where: { friendId: userId }, with: { friendSetting: true }, }, }, }, }, }, }, }); }
375-417: Complex filter logic to determine friends eligible for sharing.This code segment filters and maps friend players to determine who should receive shared content based on their settings. It's complex but necessary to enforce proper sharing restrictions.
Consider adding a comment explaining the key conditions for auto-sharing eligibility to make the logic more clear to future maintainers:
+ // Auto-share matches with friends when: + // 1. The friend has enabled auto-sharing matches (autoShareMatches) + // 2. The friend allows receiving shared matches (allowSharedMatches)
1675-1714: Consider adding error handling for edge cases in createSharedMatch.The function correctly throws an error if the shared match creation fails, but doesn't handle potential edge cases like duplicate shared matches.
Consider adding a check for existing shared matches before creation:
async function createSharedMatch( transaction: TransactionType, ownerId: number, matchId: number, shareFriend: {...}, sharedGameId: number, sharedLocationId: number | undefined, ) { + // Check for existing shared match to avoid duplicates + const existingSharedMatch = await transaction.query.sharedMatch.findFirst({ + where: { + ownerId, + sharedWithId: shareFriend.friendUserId, + matchId, + sharedGameId, + }, + }); + + if (existingSharedMatch) { + return existingSharedMatch; + } const [createdSharedMatch] = await transaction .insert(sharedMatch) .values({ // ... existing code }) .returning(); // ... remaining code }
1715-1806: Consider refactoring handlePlayerSharing for readabilityThe function is lengthy and handles multiple operations (share request creation, shared player creation, shared match player creation). This complexity makes it harder to maintain and test.
Consider breaking this function down into smaller helper functions, each with a single responsibility:
async function handlePlayerSharing(...) { // Get or create shared player if settings allow const returnedSharedPlayer = shareFriend.sharePlayers && shareFriend.allowSharedPlayers ? await getOrCreateSharedPlayer(transaction, userId, matchPlayer, shareFriend, newShareId) : null; // Create shared match player if match was shared if (returnedSharedMatch && shareFriend.autoAcceptMatches) { await createSharedMatchPlayer( transaction, userId, matchPlayer, shareFriend, returnedSharedMatch, returnedSharedPlayer ); } } // Additional helper functions...packages/api/src/routers/sharing/share-request.ts (4)
364-740: Complex friend sharing logic could benefit from additional modularizationWhile the code is now more organized than before, this match sharing implementation is still quite long (376 lines) and handles multiple concerns. Consider further modularization by extracting functionality for shared scoresheets, players, and game sharing into separate helpers similar to how you've done with
handleLocationSharing.For example, you could create helpers like:
// Extract scoresheet sharing logic async function handleScoresheetSharing( transaction: TransactionType, userId: number, friendId: number, scoresheetId: number, sharedGameId: number, autoAccept: boolean, permission: "view" | "edit", parentShareId: number, expiresAt: Date | null ) { // Implementation... } // Extract player sharing logic async function handlePlayerSharing( transaction: TransactionType, userId: number, friendId: number, playerId: number, autoAccept: boolean, permission: "view" | "edit", parentShareId: number, expiresAt: Date | null ) { // Implementation... }
1118-1118: Commented code intention without implementationThere's a comment indicating an intention to check if a match is already shared, but there's no implementation following it. Either implement the check or remove the comment to avoid confusion.
- //check if the match is already shared with the user or the previous share request has expired + // Check if the match is already shared + const existingMatchShare = await hasExistingShare( + tx2, + userId, + friendToShareTo.id, + "match", + matchToShare.matchId + ); + + if (existingMatchShare) { + shareMessages.push({ + success: false, + message: "Match already shared or pending", + }); + return false; + }
1709-1712: Share request expiration logic could be more flexibleThe current implementation only checks for pending shares created within the last 7 days. Consider making this duration configurable or moving it to a constant at the top of the file for easier maintenance.
+// At the top of the file +const PENDING_SHARE_EXPIRY_DAYS = 7; // Then in the function - createdAt: { - gt: subDays(new Date(), 7), - }, + createdAt: { + gt: subDays(new Date(), PENDING_SHARE_EXPIRY_DAYS), + },
2113-2176: Scoresheet sharing could be optimized for performanceThe
shareScoresheetsWithFriendfunction performs separate database queries for each scoresheet. Consider optimizing this by batching queries where possible or implementing bulk operations.For example, you could fetch all existing shared scoresheets in a single query:
// Fetch all existing shared scoresheets in one query const existingSharedScoresheets = await transaction.query.sharedScoresheet.findMany({ where: { scoresheetId: { in: scoreSheets.map(s => s.scoresheetId) }, sharedWithId: friendId, ownerId: userId } }); // Create a Set for O(1) lookups const existingScoreSheetIds = new Set( existingSharedScoresheets.map(s => s.scoresheetId) ); // Then check against this Set in your loop for (const scoresheetToShare of scoreSheets) { // ... existing code ... if (autoAcceptGame) { const alreadyShared = existingScoreSheetIds.has(scoresheetToShare.scoresheetId); if (!alreadyShared && returnedSharedGame) { // Create shared scoresheet... } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/api/src/routers/match.ts(5 hunks)packages/api/src/routers/sharing/share-request.ts(9 hunks)packages/api/src/utils/sharing.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
packages/api/src/utils/sharing.ts (1)
packages/db/src/client.ts (1)
TransactionType(29-29)
packages/api/src/routers/sharing/share-request.ts (3)
packages/db/src/zodSchema/index.ts (2)
selectSharedLocationSchema(105-105)selectSharedPlayerSchema(75-75)packages/api/src/utils/sharing.ts (1)
handleLocationSharing(29-118)packages/db/src/client.ts (1)
TransactionType(29-29)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: lint
🔇 Additional comments (22)
packages/api/src/utils/sharing.ts (5)
6-23: Well-designed interface for friend sharing settings.The
FriendSharingSettingsinterface is well-structured and comprehensive, covering all necessary preferences for granular sharing control. This aligns perfectly with the PR's objective to introduce per-friend sharing preferences.
29-38: Good function signature with clear typing and documentation.The function signature for
handleLocationSharingis well-defined with appropriate parameter types and default values. The TypeScript typing is thorough, and the JSDoc comment explains the purpose and return value clearly.
40-59: Efficient query for existing share requests.The query efficiently checks for both pending and accepted share requests in a single database call using the
ORcondition, which helps minimize database round trips.
61-76: Verify that auto-accept logic always finds a corresponding shared location record.When an existing share request is found and auto-accept is enabled, the code assumes a shared location record exists. However, there's a potential edge case where a share request could exist with status "accepted" but the corresponding shared location record was not created or was deleted.
Consider strengthening the recovery logic here to handle this edge case by creating a new shared location record if one is not found.
78-91: Transaction usage ensures atomicity of share request creation.The use of the transaction object for inserting the share request ensures that the operation is atomic with other database operations in the same transaction, which is essential for data integrity.
packages/api/src/routers/match.ts (3)
7-44: Good import organization with type imports separated.The imports are organized logically, with type imports separated from regular imports. The use of the
import typesyntax for type-only imports is a good practice for TypeScript as it can help with compilation performance.
328-350: Comprehensive match data retrieval for sharing preparation.The query for retrieving the created match includes all necessary related data (scoresheet, game, matchPlayers, location) needed for the sharing process, which avoids additional database queries later.
1509-1612: Well-structured function for sharing match with friends.The
shareMatchWithFriendsfunction clearly separates the sharing of different entities (match, location, game, players) and properly handles transaction management. The nested transaction pattern ensures atomicity for each friend's sharing operation.packages/api/src/routers/sharing/share-request.ts (14)
6-12: Clean use of TypeScript imports with specific typesThe import statements clearly specify the exact types needed from the database schemas, which is a good practice for maintaining type safety and keeping the code maintainable.
69-79: Correctly wrapped database operations in transactionsYou've properly wrapped all database operations within a transaction to ensure atomicity, which is crucial for maintaining data consistency when multiple operations need to succeed or fail together.
80-178: The link sharing logic has been simplified and made consistentThe refactored link sharing implementation consistently uses transactions for all database operations, making the code more reliable and maintainable.
179-206: Good extraction of friend sharing logic into a helper functionThe delegation of friend sharing logic to a dedicated helper function (
requestShareGameToFriend) significantly improves code organization and addresses the previous feedback about modularizing this complex logic.
444-456: Good reuse of the shared location handling utilityYou've properly reused the
handleLocationSharingutility function, which follows the DRY principle and ensures consistent behavior across different parts of the application.
483-614: Consistent auto-acceptance handling with appropriate verificationThis section correctly implements auto-acceptance logic for games and scoresheets, with appropriate verification of existing shares to prevent duplicates. The code properly separates share request creation from shared entity creation.
938-942: Correct error message for missing scoresheetsHere, the error message correctly matches the condition - throwing an error when no game scoresheets are found.
1551-1654: Well-structured helper function for game sharingThe
requestShareGameToFriendfunction is well-structured and handles all the necessary concerns for sharing a game with a friend. It correctly uses nested transactions, validates permissions, and delegates to other helper functions for specific tasks.
1655-1692: Clear validation of friend sharing permissionsThis helper function cleanly separates the validation logic for checking if sharing is allowed based on user preferences and friendship status. The function has a clear return signature that indicates success or failure with an appropriate message.
1693-1724: Reusable utility to check for existing sharesThe
hasExistingSharefunction encapsulates the logic for checking if an item is already shared, which is used across multiple sharing operations. This reduces code duplication and ensures consistent behavior.
1802-2004: Comprehensive match sharing implementationThe
sharedMatchWithFriendsfunction provides a complete implementation for sharing matches, including location sharing, player sharing, and appropriate permission handling. The use of early returns with error messages keeps the code clean and maintainable.
2005-2112: Well-structured match sharing handler with appropriate error checkingThe
handleMatchSharingfunction contains appropriate null checks and error handling, ensuring that shared entities are created consistently and with proper references.
24-24: Good integration with shared utility functionsImporting the
handleLocationSharingfunction from a shared utility file is a good practice that promotes code reuse across different parts of the application.
1344-1354: Efficient use of the handleMatchSharing helperThis segment properly delegates to the
handleMatchSharinghelper function, passing all required parameters for creating shared match entries. The function maintains the hierarchical relationship between games, matches, locations, and players.
…ing and transaction management - Refactor match sharing functionality to streamline the process of sharing matches and players with friends based on their settings. - Introduce a new `createOrFindSharedPlayer` function to handle player sharing more effectively, ensuring proper permission assignment and transaction management. - Update error handling and validation during share requests, improving user feedback and overall clarity in the sharing process.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/api/src/routers/match.ts (1)
1616-1676: 🛠️ Refactor suggestionAdd explicit return type for better type safety.
The
handleGameSharingfunction has an inconsistent return pattern. It returns a shared game object in some cases and null in others without a clear type definition.async function handleGameSharing( transaction: TransactionType, ownerId: number, gameId: number, shareFriend: { friendUserId: number; shareLocation: boolean; sharePlayers: boolean; defaultPermissionForMatches: "view" | "edit"; defaultPermissionForPlayers: "view" | "edit"; defaultPermissionForLocation: "view" | "edit"; defaultPermissionForGame: "view" | "edit"; allowSharedPlayers: boolean; allowSharedLocation: boolean; autoAcceptMatches: boolean; autoAcceptPlayers: boolean; autoAcceptLocation: boolean; }, newShareId: number, +): Promise<z.infer<typeof selectSharedGameSchema> | null> {packages/api/src/routers/sharing/share-request.ts (1)
939-943:⚠️ Potential issueFix error message to match the condition.
The error message indicates "No games scoresheets found" but the condition is checking for an existing share game request. This mismatch could confuse developers maintaining the code.
- throw new TRPCError({ - code: "INTERNAL_SERVER_ERROR", - message: "No games scoresheets found", - }); + throw new TRPCError({ + code: "INTERNAL_SERVER_ERROR", + message: "Expected game scoresheets not found", + });
🧹 Nitpick comments (4)
packages/api/src/routers/match.ts (3)
1677-1716: Add explicit return type definition for createSharedMatch.The function logic is sound, but adding an explicit return type would improve type safety and code clarity.
async function createSharedMatch( transaction: TransactionType, ownerId: number, matchId: number, shareFriend: { friendUserId: number; shareLocation: boolean; sharePlayers: boolean; defaultPermissionForMatches: "view" | "edit"; defaultPermissionForPlayers: "view" | "edit"; defaultPermissionForLocation: "view" | "edit"; defaultPermissionForGame: "view" | "edit"; allowSharedPlayers: boolean; allowSharedLocation: boolean; autoAcceptMatches: boolean; autoAcceptPlayers: boolean; autoAcceptLocation: boolean; }, sharedGameId: number, sharedLocationId: number | undefined, +): Promise<z.infer<typeof selectSharedMatchSchema>> {
1717-1766: Optimize player sharing with bulk operations where possible.The function correctly handles player sharing, creating share requests and shared entities as needed. For performance optimization with many players, consider implementing bulk inserts rather than iterating through each player individually.
1833-1881: Consider documenting the permissions propagation model.The
createSharedMatchPlayerfunction correctly sets up the relationship between shared matches, players, and match players with appropriate permissions. Consider adding documentation comments to clarify how permissions propagate through the sharing hierarchy.+/** + * Creates a shared match player entity, linking a shared match with a player. + * Permissions are inherited from the match-level permissions when the shared player + * doesn't exist, ensuring consistent permission propagation throughout the sharing hierarchy. + */ async function createSharedMatchPlayer(packages/api/src/routers/sharing/share-request.ts (1)
1818-2020: Consider further decomposing this lengthy function.The
sharedMatchWithFriendsfunction is quite long (200+ lines) and handles multiple responsibilities. While it works correctly, it could be decomposed into smaller functions for better maintainability and testability.For example, you could extract the player sharing logic (lines 1939-2017) into a separate function:
async function handleMatchPlayerSharing( transaction: TransactionType, userId: number, matchPlayers: any[], friendId: number, parentShare: { id: number; permission: "view" | "edit"; expiresAt: Date | null }, returnedShareMatch: any | null, friendSettings: any | undefined, ) { // Player sharing logic here }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/api/src/routers/match.ts(5 hunks)packages/api/src/routers/sharing/share-request.ts(9 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/api/src/routers/match.ts (3)
packages/db/src/client.ts (1)
TransactionType(29-29)packages/db/src/zodSchema/index.ts (1)
selectSharedLocationSchema(105-105)packages/api/src/utils/sharing.ts (1)
handleLocationSharing(29-118)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: lint
- GitHub Check: typecheck
🔇 Additional comments (8)
packages/api/src/routers/match.ts (2)
1511-1514: Well-structured match sharing implementation.The
shareMatchWithFriendsfunction follows a good pattern of orchestrating the sharing process for multiple friends. The function signature is clear and the implementation handles permissions and auto-acceptance logic correctly.
1589-1601:⚠️ Potential issueAdd a null check before using returnedSharedGame in createSharedMatch.
There's a potential issue if
handleGameSharingreturns null but thencreateSharedMatchis called with an expectation that it returned a valid object.let returnedSharedMatch: z.infer<typeof selectSharedMatchSchema> | null = null; - if (shareFriend.autoAcceptMatches && returnedSharedGame) { + // Ensure we have a valid shared game before creating a shared match + if (shareFriend.autoAcceptMatches && returnedSharedGame && returnedSharedGame.id) { returnedSharedMatch = await createSharedMatch( tx, userId, createdMatch.id, shareFriend, returnedSharedGame.id, returnedSharedLocation?.id ?? undefined, ); }Likely an incorrect or invalid review comment.
packages/api/src/routers/sharing/share-request.ts (6)
1567-1670: Well-structured helper for game sharing with friends.The
requestShareGameToFriendfunction properly encapsulates the complex logic for sharing games with friends, including validation, permission checks, and handling of child entities like matches and scoresheets.
1671-1708: Good separation of concerns for validation logic.The
validateFriendSharingPermissionsfunction is well-designed to separate validation logic from the sharing implementation, making it reusable across different sharing operations.
1709-1740: Consistent approach for checking existing shares.The
hasExistingSharefunction provides a standardized way to check for existing shares, reducing code duplication and ensuring consistent behavior across different sharing operations.
2129-2192: Well-structured scoresheet sharing implementation.The
shareScoresheetsWithFriendfunction follows a clean pattern of checking for existing shares, creating share requests, and creating shared entities if auto-accept is enabled.
69-79: Consistent use of transactions improves data integrity.The refactored code correctly uses a transaction for all database operations, ensuring atomicity and consistency when sharing entities.
2021-2128: Good error handling in match sharing logic.The
handleMatchSharingfunction includes comprehensive error handling for cases where required shared entities (like shared games) are missing, preventing inconsistent sharing states.
- Rename `linkedPlayers` to `linkedPlayer` and adjust the relationship to reference `createdBy` instead of `userId`. - Introduce filtering conditions for linked players to ensure only active users are included. - Remove the `friendPlayer` schema and its references from the database, streamlining the player relationships. - Update player schema to include `isUser` and `friendId` fields for better relationship management. - Modify seeding logic to set `isUser` for players created by users, enhancing data integrity.
- Update various routers to replace `userId` checks with the new `isUser` flag for improved clarity and consistency in user identification. - Adjust player and match handling logic to utilize the `isUser` property, enhancing data integrity and simplifying user-related queries. - Ensure all relevant areas in the codebase reflect this change, streamlining the user identification process across the application.
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
packages/api/src/routers/friends.ts (1)
164-176:clerkClientshould not be called – the SDK object is already initialised
This was flagged in a previous review but is still present. Callingawait clerkClient()throwsTypeError: clerkClient is not a function.-const client = await clerkClient(); +const client = clerkClient;
🧹 Nitpick comments (5)
apps/nextjs/src/app/dashboard/friends/[id]/page.tsx (1)
25-27: Added Open Graph metadata for better social sharing.This enhancement improves SEO and social media sharing by adding the friend's image URL to the metadata. Consider adding a null check for
friend.imageUrlto prevent errors if it's undefined.openGraph: { - images: [friend.imageUrl], + images: friend.imageUrl ? [friend.imageUrl] : [], },apps/nextjs/src/components/breadcrumbs.tsx (2)
26-35:useQueryloses type-safety by castingany– let TRPC/React-Query infer it automatically
useQuery(trpc.dashboard.getBreadCrumbs.queryOptions(/* … */))already returns a typed hook, so the subsequent cast at line 36 is unnecessary and masks type errors.-const data = BreadCrumbsQuery.data as - | RouterOutputs["dashboard"]["getBreadCrumbs"] - | undefined; +const data = BreadCrumbsQuery.data;
40-62: Duplicate URL parsing logic – extract to a helper for readability & DRYThe fallback branch repeats the same
paths.split("/").filter(...).slice(...)pattern three times, which is brittle and hard to read. Consider moving the transformation into a dedicated utility:function splitPath(pathname: string) { const parts = pathname.split("/").filter(Boolean); return parts.map((_p, i) => ({ name: parts[i] .replace(/-/g, " ") .replace(/\b\w/g, (c) => c.toUpperCase()), path: "/" + parts.slice(0, i + 1).join("/"), })); }Then simply use
splitPath(paths).slice(1)in the JSX.packages/api/src/routers/friends.ts (2)
573-576: UsecompareDesc(or simpler multiplier) for descending sort
compareAsc(b.date, a.date)works but is obscure;compareDesc(a.date, b.date)(or(b.date as any) - (a.date as any)) conveys intent directly.-allMatches.sort((a, b) => compareAsc(b.date, a.date)); +allMatches.sort((a, b) => compareDesc(a.date, b.date));
599-607: InitialwinRatecalculation should mirror the later update logicThe update path sets
g.winRate = g.wins / g.plays, but the initial insertion storeswinRate: isWin(0 or 1). This is correct numerically but obscures intent and requires mental gymnastics.- winRate: isWin, + winRate: isWin / 1, // explicit ratio for consistency
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.vscode/settings.json(1 hunks)apps/nextjs/src/app/dashboard/friends/[id]/_components/friend-profile-page.tsx(3 hunks)apps/nextjs/src/app/dashboard/friends/[id]/_components/shared-game-card.tsx(3 hunks)apps/nextjs/src/app/dashboard/friends/[id]/_components/shared-player-card.tsx(2 hunks)apps/nextjs/src/app/dashboard/friends/[id]/page.tsx(2 hunks)apps/nextjs/src/components/breadcrumbs.tsx(2 hunks)packages/api/src/root.ts(0 hunks)packages/api/src/routers/dashboard.ts(5 hunks)packages/api/src/routers/friends.ts(6 hunks)
💤 Files with no reviewable changes (1)
- packages/api/src/root.ts
✅ Files skipped from review due to trivial changes (1)
- .vscode/settings.json
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/api/src/routers/dashboard.ts
🧰 Additional context used
🧬 Code Graph Analysis (3)
apps/nextjs/src/app/dashboard/friends/[id]/_components/shared-player-card.tsx (1)
apps/nextjs/src/app/dashboard/settings/_components/uploadBGGdata.tsx (1)
Player(118-126)
apps/nextjs/src/app/dashboard/friends/[id]/_components/shared-game-card.tsx (2)
apps/expo/src/utils/api.tsx (1)
RouterOutputs(57-57)apps/nextjs/src/app/dashboard/settings/_components/uploadBGGdata.tsx (1)
Game(49-76)
packages/api/src/routers/friends.ts (2)
packages/api/src/utils/sharing.ts (2)
collectShares(172-309)mapSharedMatchPlayers(311-398)packages/api/src/trpc.ts (1)
protectedUserProcedure(182-182)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: lint
🔇 Additional comments (11)
apps/nextjs/src/app/dashboard/friends/[id]/page.tsx (2)
6-6: Corrected import path spelling.This fix resolves the typo in the directory name from "componenets" to "components".
23-23:✅ Verification successful
Check for null or undefined friend name.
The fallback value that was previously present (
?? "Friend") has been removed. Ensure thatfriend.nameis always defined, or consider adding a fallback.
🏁 Script executed:
#!/bin/bash # Check if there are any null checks for friend.name in the codebase rg "friend\.name\s*\?\?" --type=typescriptLength of output: 79
🏁 Script executed:
#!/bin/bash # Search for any remaining fallback usages of friend.name (e.g., ?? "…") rg -e 'friend\.name\s*\?\?' -g '*.ts' -g '*.tsx' # Search for optional chaining usage on friend.name rg -e 'friend\?\.name' -g '*.ts' -g '*.tsx'Length of output: 573
🏁 Script executed:
#!/bin/bash # Display the first 200 lines of the page to inspect how `friend` is obtained and typed sed -n '1,200p' apps/nextjs/src/app/dashboard/friends/\[id\]/page.tsx # Find where `getFriend` is imported and used to see the return type rg -n "import.*getFriend" apps/nextjs/src/app/dashboard/friends/\[id\]/page.tsx rg -n "getFriend" apps/nextjs/src/app/dashboard/friends/\[id\]/page.tsxLength of output: 2223
No fallback needed for
friend.nameThe
generateMetadatacall already guards against a missingfriend(if (!friend) redirect(...)), and the TRPCgetFriendMetaDataprocedure returns a non-nullablenamestring. Thereforefriend.nameis always defined and no?? "Friend"fallback is required.apps/nextjs/src/app/dashboard/friends/[id]/_components/shared-player-card.tsx (2)
16-23: Improved type definitions with Extract utility.The introduction of
SharedItemtype and use of TypeScript'sExtractutility type provides better type safety and more accurate representation of the data structure from the API.
34-34: Updated property access to match new data structure.The property access has been correctly updated from the nested
player.player.nameto the directplayer.nameproperty, aligning with the flattened data structure from the API.apps/nextjs/src/app/dashboard/friends/[id]/_components/friend-profile-page.tsx (2)
60-83: Direct iteration through shared items is more efficient.The code now directly maps over
friend.sharedToarray rather than using intermediate arrays created withuseMemo. This simplifies the code and improves performance by eliminating unnecessary array manipulations.However, add null checks for Clerk user properties in the empty state message:
<p className="mt-2 text-sm text-muted-foreground"> - {`${friend.clerkUser.firstName} ${friend.clerkUser.lastName}`}{" "} + {`${friend.clerkUser?.firstName || ""} ${friend.clerkUser?.lastName || ""}`}{" "} hasn't shared any games or players with you yet </p>
87-111: Similar improvement for shared-with-friend section.The direct use of
friend.sharedWitharray is a good optimization, similar to the shared-by-friend section.Add null checks for Clerk user properties in the empty state message here as well:
You haven't shared any games or players with{" "} - {`${friend.clerkUser.firstName} ${friend.clerkUser.lastName}`}{" "} + {`${friend.clerkUser?.firstName || ""} ${friend.clerkUser?.lastName || ""}`}{" "} yetapps/nextjs/src/app/dashboard/friends/[id]/_components/shared-game-card.tsx (5)
16-23: Consistent type pattern with shared-player-card.Good use of TypeScript's
Extractutility type to define theGametype, maintaining consistency with the approach used in shared-player-card.tsx.
34-34: Updated property access to match flattened data structure.Properly updated from the nested
game.game.nameto the directgame.nameproperty, reflecting the simplified data structure.
48-53: Updated matches array reference.The property has been correctly updated from
sharedMatchestomatches, along with proper pluralization logic.
54-59: Updated scoresheets array reference.The property has been correctly updated from
sharedScoresheetstoscoresheets, maintaining consistent pluralization logic.
61-63: Add "Game only" message when no related items are shared.This provides a clearer user experience by explicitly stating that only the game itself is shared when there are no matches or scoresheets included.
apps/nextjs/src/app/dashboard/friends/[id]/_components/friend-profile-page.tsx
Outdated
Show resolved
Hide resolved
apps/nextjs/src/app/dashboard/friends/[id]/_components/friend-profile-page.tsx
Outdated
Show resolved
Hide resolved
apps/nextjs/src/app/dashboard/friends/[id]/_components/friend-profile-page.tsx
Outdated
Show resolved
Hide resolved
- Refactor the friends router to utilize a new `mapMatches` utility for improved handling of match data, streamlining the retrieval process for both original and shared matches. - Simplify the logic for mapping clerk user details to friends, enhancing clarity and maintainability. - Remove redundant match record definitions and improve the structure of match data retrieval, ensuring a more efficient and readable codebase. - Update the sharing logic to reflect changes in data structure, enhancing the overall sharing experience.
- Update the friend profile page to use a unified name format for clerk users, improving consistency in display. - Modify metadata generation to handle cases where the friend's image URL may be absent, ensuring robust metadata creation. - Streamline breadcrumb component by simplifying query handling and removing unnecessary type definitions, enhancing clarity and performance.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/api/src/utils/game.ts (1)
264-264: UsecompareDescfor clarity & intentYou rely on the argument reversal trick (
compareAsc(b.date, a.date)) to get descending order.
While it works, it obscures intent for the next maintainer.date-fnsexposescompareDesc, which is self-documenting and avoids cognitive overhead.-import { compareAsc } from "date-fns"; +import { compareDesc } from "date-fns"; ... -allMatches.sort((a, b) => compareAsc(b.date, a.date)); +allMatches.sort((a, b) => compareDesc(a.date, b.date));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/nextjs/src/app/dashboard/friends/[id]/_components/friend-profile-page.tsx(3 hunks)apps/nextjs/src/app/dashboard/friends/[id]/page.tsx(2 hunks)apps/nextjs/src/components/breadcrumbs.tsx(2 hunks)packages/api/src/routers/friends.ts(8 hunks)packages/api/src/utils/game.ts(1 hunks)packages/api/src/utils/sharing.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- apps/nextjs/src/app/dashboard/friends/[id]/page.tsx
- apps/nextjs/src/app/dashboard/friends/[id]/_components/friend-profile-page.tsx
- apps/nextjs/src/components/breadcrumbs.tsx
- packages/api/src/utils/sharing.ts
- packages/api/src/routers/friends.ts
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: lint
🔇 Additional comments (1)
packages/api/src/utils/game.ts (1)
308-319: Filtering may remove non-friend players unintentionally
filter(p => p.sharedPlayer?.linkedPlayerId === friendPlayerId)only keeps players explicitly linked to the friend, so opponents / team-mates disappear from theplayerslist of shared matches.
If the UI needs the full roster (mirroring behaviour of original matches), drop this filter and instead flag the friend withisUser.Please confirm whether the consumer expects all players or only the friend’s representation. If the former, we need to adjust the filter logic.
- Update the FriendsPage component to improve layout and responsiveness, utilizing a grid system for better organization of friend lists and requests. - Introduce skeleton loading components for FriendsList and FriendRequests to enhance user experience during data fetching. - Refactor FriendCard and FriendRequestCard components to accept structured friend and request data, improving clarity and maintainability. - Remove the deprecated friend-tab component, consolidating functionality into more focused components for better code organization.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
packages/api/src/utils/game.ts (2)
233-259:⚠️ Potential issuePotential duplication of the same shared match
The nested forEach loops can result in duplicate matches being added to
allMatchesif a match contains multiplesharedMatchPlayersrecords.sharedLinkedPlayers.forEach((lp) => { + const seen = new Set<number>(); lp.sharedMatchPlayers.forEach((smp) => { + if (seen.has(smp.sharedMatch.match.id)) return; // skip duplicates + seen.add(smp.sharedMatch.match.id); const sm = smp.sharedMatch; const gameEntity = sm.sharedGame.linkedGame ?? sm.sharedGame.game; const players = mapSharedMatchPlayers(sm.sharedMatchPlayers); const locationName = sm.sharedLocation?.linkedLocation ? sm.sharedLocation.linkedLocation.name : sm.sharedLocation?.location.name; allMatches.push({ type: "Shared", id: sm.match.id, name: sm.match.name, date: sm.match.date, duration: sm.match.duration, finished: sm.match.finished, gameId: gameEntity.id, gameName: gameEntity.name, locationName: locationName ?? undefined, players, outcome: { score: smp.matchPlayer.score, isWinner: smp.matchPlayer.winner ?? false, placement: smp.matchPlayer.placement, }, }); }); });
302-324:⚠️ Potential issueLoss of type-safety after filtering nulls
TypeScript doesn't automatically narrow the type after filtering out null values without a type predicate. Fix this by adding an explicit type predicate to the filter.
return sharedMatchPlayers .map((p) => { const linkedPlayer = p.sharedPlayer?.linkedPlayer; if (linkedPlayer) { return { playerId: linkedPlayer.id, name: linkedPlayer.name, score: p.matchPlayer.score, isWinner: p.matchPlayer.winner ?? false, placement: p.matchPlayer.placement, }; } else if (p.sharedPlayer) { return { playerId: p.sharedPlayer.playerId, name: p.sharedPlayer.player.name, score: p.matchPlayer.score, isWinner: p.matchPlayer.winner ?? false, placement: p.matchPlayer.placement, }; } return null; }) - .filter((p) => p !== null); + .filter( + (p): p is NonNullable<typeof p> => p !== null, + );packages/api/src/routers/friends.ts (2)
712-722:⚠️ Potential issueFix the clerkClient usage
clerkClientis an object, not a function, so it shouldn't be called with parentheses.- const client = await clerkClient(); + const client = clerkClient; const clerkUser = await client.users .getUser(returnedFriend.friend.clerkUserId) .catch((error) => { console.error(error); throw new TRPCError({ code: "NOT_FOUND", message: "Friend not found", }); });
865-875:⚠️ Potential issueFix clerkClient usage in getFriendMetaData
This is the same issue as above -
clerkClientis an object, not a function.- const client = await clerkClient(); + const client = clerkClient; const clerkUser = await client.users .getUser(returnedFriend.friend.clerkUserId) .catch((error) => { console.error(error); throw new TRPCError({ code: "NOT_FOUND", message: "Friend not found", }); });
🧹 Nitpick comments (6)
apps/nextjs/src/app/dashboard/friends/_components/friend-requests-skeleton.tsx (1)
27-57: Consider implementing skeleton for both tabs.The skeleton only implements the "Received" tab content and not the "Sent" tab. For a more complete loading experience, consider adding skeleton placeholders for the "Sent" tab as well.
+ <TabsContent value="sent"> + <Card> + <CardHeader> + <CardTitle>Sent Requests</CardTitle> + <CardDescription>Friend requests you've sent</CardDescription> + </CardHeader> + <CardContent> + <div className="space-y-4"> + {Array(2) + .fill(0) + .map((_, i) => ( + <div key={i} className="rounded-lg border p-4"> + <div className="flex items-center gap-4"> + <Skeleton className="h-10 w-10 rounded-full" /> + <div className="flex-grow space-y-2"> + <Skeleton className="h-4 w-32" /> + <Skeleton className="h-3 w-24" /> + </div> + <div className="flex gap-2"> + <Skeleton className="h-8 w-8 rounded-full" /> + </div> + </div> + </div> + ))} + </div> + </CardContent> + </Card> + </TabsContent>apps/nextjs/src/app/dashboard/friends/_components/friend-card.tsx (1)
76-96: Enhanced user profile display with accessibility improvements.The updated layout with Avatar component and additional user details (username/email) provides a more comprehensive and visually appealing display. Consider adding an aria-label to the link for better accessibility.
<Link prefetch={true} href={`/dashboard/friends/${friend.id}`} className="flex items-center gap-4" + aria-label={`View ${friend.name}'s profile`} >apps/nextjs/src/app/dashboard/friends/_components/friend-requests-tabs.tsx (1)
67-80: Add pagination for potentially large request lists.The scrollable area with fixed max height is a good start, but consider implementing pagination for better performance with large numbers of friend requests.
// Example pagination implementation const [page, setPage] = useState(1); const pageSize = 10; const paginatedRequests = pendingRequests.slice((page - 1) * pageSize, page * pageSize); // In the JSX, render paginatedRequests instead of pendingRequests // Then add pagination controls below: <div className="mt-4 flex justify-center"> <Pagination> <PaginationContent> <PaginationItem> <PaginationPrevious onClick={() => setPage(p => Math.max(1, p - 1))} disabled={page === 1} /> </PaginationItem> {/* Page numbers */} <PaginationItem> <PaginationNext onClick={() => setPage(p => p + 1)} disabled={page * pageSize >= pendingRequests.length} /> </PaginationItem> </PaginationContent> </Pagination> </div>apps/nextjs/src/app/dashboard/friends/_components/friend-list.tsx (2)
28-38: Consider adding TypeScript type for filtered friendsThe
filteredFriendsfunction uses implicit typing for the friend objects. Adding explicit type definitions would improve type safety and make the component more maintainable.const filteredFriends = useMemo(() => { const query = searchQuery.toLowerCase(); + // Define explicit return type return friends.filter((friend) => { const name = friend.name; const email = friend.email ?? ""; return ( name.toLowerCase().includes(query) || email.toLowerCase().includes(query) ); }); }, [friends, searchQuery]);
59-67: Consider adding pagination for large friends listsFor users with many friends, the current implementation loads and displays all friends at once, which could lead to performance issues. Consider implementing pagination or virtual scrolling for better performance.
{filteredFriends.length > 0 ? ( <ScrollArea> - <div className="grid max-h-[600px] gap-2"> + <div className="grid max-h-[600px] gap-2"> {filteredFriends.map((friend) => ( <FriendCard key={friend.id} friend={friend} /> ))} </div> </ScrollArea>apps/nextjs/src/app/dashboard/friends/_components/friend-request-card.tsx (1)
80-102: Consider extracting reusable success handler logicThe success handlers for all three mutations (accept, reject, cancel) perform the same query invalidation steps. Consider extracting this logic to a reusable function to reduce duplication.
+ // Add at the top of the component + const handleSuccessfulMutation = async (message: string) => { + toast({ title: message }); + await queryClient.invalidateQueries( + trpc.friend.getFriendRequests.queryOptions(), + ); + await queryClient.invalidateQueries( + trpc.friend.getFriends.queryOptions(), + ); + setIsLoading(null); + }; const cancelFriendRequestMutation = useMutation( trpc.friend.cancelFriendRequest.mutationOptions({ - onSuccess: async () => { - toast({ - title: "Friend request canceled", - }); - await queryClient.invalidateQueries( - trpc.friend.getFriendRequests.queryOptions(), - ); - await queryClient.invalidateQueries( - trpc.friend.getFriends.queryOptions(), - ); - setIsLoading(null); - }, + onSuccess: () => handleSuccessfulMutation("Friend request canceled"), onError: (error) => { toast({ title: "Error", description: error.message || "Failed to cancel friend request", variant: "destructive", }); }, }), );Then apply similar changes to the other mutations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
apps/nextjs/src/app/dashboard/friends/_components/friend-card.tsx(6 hunks)apps/nextjs/src/app/dashboard/friends/_components/friend-list-skeleton.tsx(1 hunks)apps/nextjs/src/app/dashboard/friends/_components/friend-list.tsx(1 hunks)apps/nextjs/src/app/dashboard/friends/_components/friend-request-card.tsx(2 hunks)apps/nextjs/src/app/dashboard/friends/_components/friend-requests-skeleton.tsx(1 hunks)apps/nextjs/src/app/dashboard/friends/_components/friend-requests-tabs.tsx(1 hunks)apps/nextjs/src/app/dashboard/friends/_components/friend-tab.tsx(0 hunks)apps/nextjs/src/app/dashboard/friends/page.tsx(3 hunks)packages/api/src/routers/friends.ts(10 hunks)packages/api/src/utils/game.ts(1 hunks)
💤 Files with no reviewable changes (1)
- apps/nextjs/src/app/dashboard/friends/_components/friend-tab.tsx
✅ Files skipped from review due to trivial changes (1)
- apps/nextjs/src/app/dashboard/friends/_components/friend-list-skeleton.tsx
🧰 Additional context used
🧬 Code Graph Analysis (1)
apps/nextjs/src/app/dashboard/friends/page.tsx (4)
apps/nextjs/src/app/dashboard/friends/_components/friend-list-skeleton.tsx (1)
FriendsListSkeleton(14-51)apps/nextjs/src/app/dashboard/friends/_components/friend-list.tsx (1)
FriendsList(21-79)apps/nextjs/src/app/dashboard/friends/_components/friend-requests-skeleton.tsx (1)
FriendRequestsSkeleton(13-60)apps/nextjs/src/app/dashboard/friends/_components/friend-requests-tabs.tsx (1)
FriendRequestsTabs(20-112)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: lint
- GitHub Check: typecheck
🔇 Additional comments (15)
apps/nextjs/src/app/dashboard/friends/_components/friend-requests-skeleton.tsx (2)
1-11: Appropriate icon and UI imports for the skeleton component.Good selection of imports for the skeleton component, matching the icons and UI components that would be used in the actual
FriendRequestsTabscomponent.
13-25: Well-structured tabs header with matching layout to actual component.The skeleton tabs implementation correctly mirrors the structure in the actual
FriendRequestsTabscomponent, including the grid layout and icon placement. This provides a seamless transition when content loads.apps/nextjs/src/app/dashboard/friends/_components/friend-card.tsx (4)
8-9: Good type imports and UI component updates.The changes appropriately import the necessary types from the API and UI components required for the enhanced friend card.
Also applies to: 19-21
33-35: Improved prop structure with type safety.Refactoring the props to use the API response type provides better type safety and makes the component more maintainable as the friend data structure evolves.
54-56: Enhanced query invalidation for better data consistency.Good improvement to invalidate both the friends list and the specific friend query, ensuring UI consistency across the application when a friend is removed.
132-133: Improved dialog text with dynamic friend name.Using the friend's name in the dialog text makes the confirmation more personalized and clear to the user.
apps/nextjs/src/app/dashboard/friends/_components/friend-requests-tabs.tsx (4)
1-19: Properly structured client component with necessary imports.The component correctly uses client-side rendering and imports all the necessary dependencies for state management, data fetching, and UI rendering.
20-28: Well-implemented state management and data fetching.The component uses React state for managing the active tab and correctly implements suspense queries for data fetching. This allows for a clean component structure without explicit loading states.
30-55: Dynamic badge counts for improved user experience.The tabs implementation with dynamic badge counts for both received and sent requests provides useful information at a glance. This is a great UX enhancement.
93-106: Consistent UI patterns between tabs.Good job maintaining consistency between the "Received" and "Sent" tabs UI patterns, including the scrollable container, space styling, and empty state messages.
apps/nextjs/src/app/dashboard/friends/page.tsx (3)
4-7: Properly modularized component imports.The updated imports reflect the more modular component structure, with separate components for the friends list, friend requests, and their respective skeleton loaders.
16-24: Improved responsive header layout.The updated header layout with flex-col to flex-row transition at the md breakpoint provides better mobile support.
26-38: Enhanced responsive layout with individual loading states.The updated grid layout with separate Suspense boundaries for each component is excellent. It provides a better user experience by:
- Allowing partial content to load independently
- Using specific skeleton components for each section
- Creating a better layout on both small and large screens with the grid-cols-1 to grid-cols-3 transition
This is a significant improvement over the previous implementation.
packages/api/src/routers/friends.ts (2)
119-123: Batch fetching Clerk users for better performanceYou're correctly using
getUserListto batch fetch Clerk users, which is more efficient than making individual requests for each user.
772-809: Consider using a Map to avoid duplicate game processingYour implementation for building gameMap is efficient, using a Map to track unique games and aggregate statistics. This is a good pattern for avoiding duplicates and maintaining efficient lookups.
apps/nextjs/src/app/dashboard/friends/_components/friend-request-card.tsx
Outdated
Show resolved
Hide resolved
…ality - Introduce a new FriendProfileSkeleton component for improved loading states on friend profile pages. - Refactor FriendProfilePage to utilize the new skeleton component, enhancing user experience during data fetching. - Implement FriendSettingsDialog and FriendSettingsForm components to manage friend-specific settings, improving user interaction and customization. - Create FriendSharedItemsPage and related components to display shared items between friends, enhancing the overall functionality of the friends dashboard. - Remove deprecated SharedGameCard and SharedPlayerCard components, streamlining the codebase and focusing on new shared items implementation.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
apps/nextjs/src/app/dashboard/friends/[id]/_components/friend-profile-page.tsx (1)
28-31: Add null checks for Clerk user propertiesThe code directly accesses Clerk user properties without null checks, which could lead to runtime errors if these properties are undefined.
<Avatar className="h-16 w-16"> <AvatarImage - src={friend.clerkUser.imageUrl} - alt={friend.clerkUser.name} + src={friend.clerkUser?.imageUrl} + alt={friend.clerkUser?.name || ""} /> <AvatarFallback>packages/api/src/routers/friends.ts (3)
119-119:⚠️ Potential issue
clerkClientis not a function – fix all call sites.The
clerkClientis already initialized and doesn't need to be called with parentheses.-const client = await clerkClient(); +const client = clerkClient;Apply this fix to all instances at lines 119, 179, 238, 712, and 868.
Also applies to: 179-179, 238-238, 712-712, 868-868
141-155: 🛠️ Refactor suggestionExtract getFullName into a reusable helper function.
The
getFullNamefunction is defined multiple times throughout this file with identical logic.Create a utility function at the top of the file:
+const getFullNameFromClerkUser = (clerkUser: any) => { + if (clerkUser.fullName) { + return clerkUser.fullName; + } + if (clerkUser.firstName && clerkUser.lastName) { + return `${clerkUser.firstName} ${clerkUser.lastName}`; + } + if (clerkUser.firstName) { + return clerkUser.firstName; + } + if (clerkUser.lastName) { + return clerkUser.lastName; + } + return "Unknown"; +};Then replace each local implementation with a call to this utility function.
Also applies to: 201-215, 258-272, 740-754, 879-893
124-132:⚠️ Potential issueType mismatch:
createdAtis included in the returned objects but missing in the inline type declarations.The mapped objects include
createdAt(returned at lines 164, 224, 280) but some type declarations omit it.Add
createdAtto all type declarations:type MappedRequest = { id: number; status: "pending" | "accepted" | "rejected"; name: string; userName: string | null; email: string | null; imageUrl: string | null; + createdAt: Date; };Consider defining these types once at the top of the file and reusing them to avoid inconsistencies.
Also applies to: 184-192, 242-249
🧹 Nitpick comments (12)
apps/nextjs/src/app/dashboard/friends/[id]/_components/friend-settings-dialog.tsx (2)
57-58: Consider increasing the timeout value for better UXThe current 10ms timeout before closing the dialog may not be sufficient for the toast notification to be properly displayed or noticed by users. This might lead to a jarring experience where the dialog closes before users can see feedback.
- setTimeout(() => setOpen(false), 10); + setTimeout(() => setOpen(false), 300);
69-69: Consider using max-height instead of fixed height for better responsivenessThe dialog has a fixed height of 750px which might cause issues on smaller screens. Using max-height with an appropriate overflow behavior would be more responsive.
- <DialogContent className="h-[750px] sm:max-w-3xl"> + <DialogContent className="max-h-[80vh] overflow-y-auto sm:max-w-3xl">apps/nextjs/src/app/dashboard/friends/[id]/_components/friend-profile-page.tsx (2)
33-35: Extract avatar initials logic to a helper functionThe complex logic for generating initials is embedded directly in the component, making it harder to read and test. Consider extracting this to a reusable helper function.
+ <AvatarFallback> + {getInitials(friend.clerkUser?.name || "")} + </AvatarFallback> - <AvatarFallback> - {(friend.clerkUser.name.split(" ")[0]?.substring(0, 1) ?? "") + - friend.clerkUser.name.split(" ")[1]?.substring(0, 1)} - </AvatarFallback>You could add this helper function at the bottom of the file:
function getInitials(name: string): string { const parts = name.split(" "); const firstInitial = parts[0]?.substring(0, 1) || ""; const lastInitial = parts[1]?.substring(0, 1) || ""; return firstInitial + lastInitial; }
153-172: Consider moving calculateHIndex to a utility fileThe H-index calculation logic would be more reusable and testable if moved to a shared utility file, especially if it's used in multiple places.
This utility function contains domain-specific logic that could be reused in other components that display player statistics. Moving it to a shared location (like a utils folder) would improve code organization and testability.
apps/nextjs/src/app/dashboard/friends/[id]/_components/friend-settings-form.tsx (2)
94-134: Simplify form initialization with spread operatorThe form initialization is verbose with all fields explicitly listed. You could simplify this using the spread operator, assuming the type safety is maintained.
const form = useForm<z.infer<typeof formSchema>>({ resolver: zodResolver(formSchema), - defaultValues: initialSettings - ? { - autoShareMatches: initialSettings.autoShareMatches, - sharePlayersWithMatch: initialSettings.sharePlayersWithMatch, - includeLocationWithMatch: initialSettings.includeLocationWithMatch, - defaultPermissionForMatches: - initialSettings.defaultPermissionForMatches, - defaultPermissionForPlayers: - initialSettings.defaultPermissionForPlayers, - defaultPermissionForLocation: - initialSettings.defaultPermissionForLocation, - defaultPermissionForGame: initialSettings.defaultPermissionForGame, - autoAcceptMatches: initialSettings.autoAcceptMatches, - autoAcceptPlayers: initialSettings.autoAcceptPlayers, - autoAcceptLocation: initialSettings.autoAcceptLocation, - autoAcceptGame: initialSettings.autoAcceptGame, - allowSharedGames: initialSettings.allowSharedGames, - allowSharedPlayers: initialSettings.allowSharedPlayers, - allowSharedLocation: initialSettings.allowSharedLocation, - allowSharedMatches: initialSettings.allowSharedMatches, - } + defaultValues: initialSettings + ? { ...initialSettings } : { autoShareMatches: false, sharePlayersWithMatch: false, // ... rest of default values
177-325: Extract repeated form field patterns to a reusable componentThere's significant duplication in the form field structure across all tabs. Consider creating a reusable component for toggle settings and permission selects to reduce code duplication.
You could create a component like:
function ToggleSetting({ name, label, description, control, disabled = false, }: { name: Path<z.infer<typeof formSchema>>; label: string; description: string; control: Control<z.infer<typeof formSchema>>; disabled?: boolean; }) { return ( <FormField control={control} name={name} render={({ field }) => ( <FormItem className="flex flex-row items-center justify-between rounded-lg border p-3"> <div className="space-y-0.5"> <FormLabel className="text-sm">{label}</FormLabel> <FormDescription className="text-xs">{description}</FormDescription> </div> <FormControl> <Switch checked={field.value} onCheckedChange={field.onChange} disabled={disabled} /> </FormControl> </FormItem> )} /> ); }This would significantly reduce repetition in your form.
apps/nextjs/src/app/dashboard/friends/[id]/shared/_components/friend-shared-items.tsx (3)
51-64: Extract duplicated search input to a reusable componentThe search input component is duplicated in both tab contents. Consider extracting it to a reusable component to reduce duplication and improve maintainability.
function SearchInput({ value, onChange, placeholder = "Search shared items..." }: { value: string; onChange: (e: React.ChangeEvent<HTMLInputElement>) => void; placeholder?: string; }) { return ( <div className="relative mb-4"> <Search className="absolute left-2 top-2.5 h-4 w-4 text-muted-foreground" /> <Input placeholder={placeholder} className="pl-8" value={value} onChange={onChange} /> </div> ); }Then use it like:
<SearchInput value={withSearchTerm} onChange={(e) => setWithSearchTerm(e.target.value)} />Also applies to: 66-79
94-119: Optimize item filtering for better performanceThe filtering logic for items is complex and might be inefficient for large datasets. Consider optimizing this by:
- Memoizing the lowercase search term
- Using early returns for simple cases
- Using a simpler loop structure
const filteredItems = useMemo(() => { if (!searchTerm) return items; const lowerSearchTerm = searchTerm.toLowerCase(); + + // Quick check for empty search term + if (lowerSearchTerm === '') return items; return items.filter((item) => { // Check if item name matches search term if (item.name.toLowerCase().includes(lowerSearchTerm)) return true; // For game items, also check matches and scoresheets if (item.type === "game") { - // Check if any match name matches search term - const matchesMatch = item.matches.some((match) => - match.name.toLowerCase().includes(lowerSearchTerm), - ); - if (matchesMatch) return true; - - // Check if any scoresheet name matches search term - const matchesScoresheet = item.scoresheets.some((sheet) => - sheet.name.toLowerCase().includes(lowerSearchTerm), - ); - if (matchesScoresheet) return true; + // Check match names and scoresheet names in a single pass + for (const match of item.matches) { + if (match.name.toLowerCase().includes(lowerSearchTerm)) return true; + } + + for (const sheet of item.scoresheets) { + if (sheet.name.toLowerCase().includes(lowerSearchTerm)) return true; + } } return false; }); }, [items, searchTerm]);This approach might be more efficient for larger datasets by avoiding multiple iterations through the arrays.
180-188: Improve type safety with generic TypeScript utilityThe type extraction is verbose and could be simplified with TypeScript utilities.
function GameItem({ item, searchTerm, }: { - item: Extract< - RouterOutputs["friend"]["getFriend"]["sharedWith"][number], - { type: "game" } - >; + item: RouterItemType<"game">; searchTerm: string; }) {You could add this utility type at the top of the file:
type RouterItemType<T extends string> = Extract< RouterOutputs["friend"]["getFriend"]["sharedWith"][number], { type: T } >;packages/api/src/routers/friends.ts (3)
737-737: Remove debug console.log statement.There's a
console.logstatement that should be removed before deploying to production.-console.log(returnedFriend);
12-21: Type definition for game aggregation.The
GameAggtype provides clear structure for game statistics, but consider using a more specific discriminated union for thetypefield.// eslint-disable-next-line @typescript-eslint/consistent-type-definitions type GameAgg = { - type: "Original" | "Shared"; + type: "Original" | "Shared"; // Consider making this a discriminated union if properties differ by type id: number; name: string; imageUrl: string | null; plays: number; wins: number; winRate: number; };
778-813: Game statistics calculation logic.The code effectively calculates game statistics by aggregating match data, but consider extracting this logic to a separate utility function for better reusability.
This complex aggregation logic could be reused elsewhere in the application. Consider moving it to a utility function such as
aggregateGameStats(matches, playerMatchData)in a separate file.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (12)
apps/nextjs/src/app/dashboard/friends/[id]/_components/friend-profile-page.tsx(1 hunks)apps/nextjs/src/app/dashboard/friends/[id]/_components/friend-profile-skeleton.tsx(1 hunks)apps/nextjs/src/app/dashboard/friends/[id]/_components/friend-settings-dialog.tsx(1 hunks)apps/nextjs/src/app/dashboard/friends/[id]/_components/friend-settings-form.tsx(1 hunks)apps/nextjs/src/app/dashboard/friends/[id]/page.tsx(3 hunks)apps/nextjs/src/app/dashboard/friends/[id]/shared/_components/friend-shared-items-page.tsx(1 hunks)apps/nextjs/src/app/dashboard/friends/[id]/shared/_components/friend-shared-items.tsx(1 hunks)apps/nextjs/src/app/dashboard/friends/[id]/shared/page.tsx(1 hunks)apps/nextjs/src/app/dashboard/friends/[id]/stats/_components/friend-stat-page.tsx(1 hunks)apps/nextjs/src/app/dashboard/friends/[id]/stats/page.tsx(1 hunks)apps/nextjs/src/app/dashboard/games/[id]/stats/_components/game-stats.tsx(1 hunks)packages/api/src/routers/friends.ts(10 hunks)
✅ Files skipped from review due to trivial changes (2)
- apps/nextjs/src/app/dashboard/friends/[id]/_components/friend-profile-skeleton.tsx
- apps/nextjs/src/app/dashboard/friends/[id]/shared/page.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/nextjs/src/app/dashboard/friends/[id]/page.tsx
🧰 Additional context used
🧬 Code Graph Analysis (6)
apps/nextjs/src/app/dashboard/games/[id]/stats/_components/game-stats.tsx (1)
packages/shared/src/utils.ts (1)
getOrdinalSuffix(21-35)
apps/nextjs/src/app/dashboard/friends/[id]/stats/page.tsx (2)
apps/nextjs/src/app/dashboard/friends/[id]/_components/friend-profile-skeleton.tsx (1)
FriendProfileSkeleton(4-40)apps/nextjs/src/app/dashboard/friends/[id]/stats/_components/friend-stat-page.tsx (1)
FriendStatsPage(19-90)
apps/nextjs/src/app/dashboard/friends/[id]/stats/_components/friend-stat-page.tsx (2)
packages/ui/src/avatar.tsx (3)
Avatar(50-50)AvatarImage(50-50)AvatarFallback(50-50)packages/shared/src/utils.ts (1)
formatDuration(14-19)
apps/nextjs/src/app/dashboard/friends/[id]/shared/_components/friend-shared-items.tsx (6)
apps/expo/src/utils/api.tsx (1)
RouterOutputs(57-57)packages/ui/src/tabs.tsx (4)
Tabs(55-55)TabsList(55-55)TabsTrigger(55-55)TabsContent(55-55)apps/expo/src/lib/icons/Dices.tsx (1)
Dices(6-6)packages/ui/src/badge.tsx (1)
Badge(37-37)apps/expo/src/lib/icons/ChevronDown.tsx (1)
ChevronDown(6-6)apps/expo/src/lib/icons/ChevronRight.tsx (1)
ChevronRight(6-6)
apps/nextjs/src/app/dashboard/friends/[id]/_components/friend-profile-page.tsx (2)
apps/nextjs/src/app/dashboard/friends/[id]/_components/friend-settings-dialog.tsx (1)
FriendSettingsDialog(29-110)packages/shared/src/utils.ts (1)
formatDuration(14-19)
packages/api/src/routers/friends.ts (3)
packages/api/src/trpc.ts (1)
protectedUserProcedure(182-182)packages/api/src/utils/sharing.ts (1)
collectShares(164-293)packages/api/src/utils/game.ts (1)
mapMatches(3-262)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: lint
🔇 Additional comments (9)
apps/nextjs/src/app/dashboard/games/[id]/stats/_components/game-stats.tsx (1)
320-327: Removed optional chaining in rendering conditionalThe optional chaining (
?.) has been removed from subsequent accesses toteamPlayers[0].placementafter the initial check. This is appropriate since the first condition already verifies thatplacementexists onteamPlayers[0], making the optional chaining redundant in the following expressions.apps/nextjs/src/app/dashboard/friends/[id]/stats/page.tsx (1)
1-25: LGTM: Well-structured server component with proper error handlingThis new server component follows best practices:
- Properly validates the ID parameter and redirects on invalid input
- Uses Suspense for asynchronous data loading with an appropriate fallback
- Maintains a clean separation between server and client components
The component structure aligns well with the Next.js App Router pattern.
apps/nextjs/src/app/dashboard/friends/[id]/shared/_components/friend-shared-items-page.tsx (1)
1-45: LGTM: Well-implemented shared items componentThis client component correctly:
- Fetches friend data using suspense query
- Displays the friend's profile with avatar and fallback
- Passes the appropriate shared items data to the child component
The component follows the project's UI patterns and properly handles data flow.
apps/nextjs/src/app/dashboard/friends/[id]/stats/_components/friend-stat-page.tsx (1)
1-90: LGTM: Comprehensive friend stats component with good error handlingThis component effectively:
- Handles the case when a friend has no linked player by redirecting
- Displays well-structured statistics with appropriate visual hierarchy
- Uses proper formatting for duration with the shared utility function
- Implements a responsive grid layout for different screen sizes
The component follows the project's UI patterns and integrates well with the friend management feature set.
apps/nextjs/src/app/dashboard/friends/[id]/_components/friend-settings-form.tsx (1)
117-133: Inconsistent default values may lead to confusionThe default values have
autoAcceptGameset totruewhile other auto-accept fields arefalse. Similarly, all "allow" fields default totrue. Ensure this difference is intentional and documented for clarity.Review whether this difference in defaults is by design or if it should be consistent.
packages/api/src/routers/friends.ts (4)
121-123: N + 1 query optimization - batch-fetching Clerk users.You've improved the code by using
getUserListto batch-fetch users instead of individual API calls. This is a good performance optimization.Also applies to: 181-183, 239-241
774-777: Good use of utility functions for match mapping.Using the
mapMatchesutility function for consistent match mapping across the application improves code reusability and maintainability.
725-736: Efficient shared entity collection.Using the
collectSharesutility to gather shared entities in both directions (from user to friend and friend to user) streamlines the code and ensures consistent data structure.
902-950: Complete implementation of friend settings management.The new
updateFriendSettingsprocedure provides comprehensive controls for friend-specific sharing preferences, supporting the PR's goal of introducing granular sharing controls.Consider adding validation to ensure the friend actually belongs to the user before allowing settings updates. The current implementation checks this, but adding a comment explaining the security check would improve code clarity.
- Update FriendStatsPage to include HydrateClient for improved data handling. - Refactor GameItem and SimpleItem components to apply consistent badge styling for shared items. - Introduce GameDetails component to display detailed statistics of games played by friends, enhancing user experience. - Improve match summary links to ensure proper identification of shared matches. - Update API logic to handle game data more efficiently, including image handling and match type differentiation.
…est handling - Update the friends router to utilize a new `getFullName` utility for improved clarity in retrieving user names from clerk user data. - Refactor the FriendRequestCard component to handle loading states more accurately, enhancing user experience during friend request interactions. - Remove redundant full name logic from the friends router, simplifying the codebase and improving maintainability.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
packages/api/src/utils/game.ts (1)
314-336: 🛠️ Refactor suggestionAdd type predicate to filter for improved type safety.
After mapping shared match players, the
.filter((p) => p !== null)lacks a type predicate, which can cause TypeScript to still treat elements as potentially null.- .filter((p) => p !== null); + .filter((p): p is NonNullable<typeof p> => p !== null);packages/api/src/routers/friends.ts (1)
120-120:⚠️ Potential issueFix incorrect usage of clerkClient.
clerkClientis an object, not a function. Remove the function call operator to avoid runtime errors.-const client = await clerkClient(); +const client = clerkClient;This same issue occurs in both locations where clerkClient is used.
Also applies to: 668-668
🧹 Nitpick comments (6)
apps/nextjs/next.config.js (1)
19-20: Remove the duplicatepicsum.photosentry.You've added
cdn.jsdelivr.netwhich is good for loading external images, butpicsum.photosis already included in line 17, making the entry on line 20 a duplicate.{ hostname: "cdn.jsdelivr.net" }, - { hostname: "picsum.photos" },apps/nextjs/src/app/dashboard/friends/[id]/stats/_components/game-details.tsx (2)
126-133: Consider keeping the "Shared" badge visible on mobile.The "Shared" badge is currently hidden on mobile devices (
hidden sm:block), which removes important context for users on smaller screens. Consider making this visible on all devices, possibly with a more compact design for mobile.<Badge variant="outline" - className="hidden bg-blue-600 text-xs text-white sm:block" + className="bg-blue-600 text-xs text-white" > Shared </Badge>
104-143: Add empty state handling for when no games are available.The component doesn't handle the case when
sortedGamesis empty. Consider adding a message or placeholder when there are no games to display.<TableBody> + {sortedGames.length === 0 ? ( + <TableRow> + <TableCell colSpan={4} className="h-24 text-center"> + No games found. + </TableCell> + </TableRow> + ) : ( {sortedGames.map((game) => { return ( <TableRow key={`${game.id}-${game.type}`}> {/* existing content */} </TableRow> ); })} + )} </TableBody>packages/api/src/utils/game.ts (1)
272-273: Consider more detailed sorting criteria.The current sorting uses simple date comparison. For matches occurring on the same date, consider adding secondary sorting criteria like match name or ID for more stable sorting.
- allMatches.sort((a, b) => compareAsc(b.date, a.date)); + allMatches.sort((a, b) => { + const dateComparison = compareAsc(b.date, a.date); + if (dateComparison !== 0) return dateComparison; + // Secondary sort by name for stable sorting of same-day matches + return a.name.localeCompare(b.name); + });packages/api/src/routers/friends.ts (2)
693-693: Remove debug console.log statement.There's a console.log statement that should be removed before production deployment.
-console.log(returnedFriend);
755-756: Consider sorting game statistics for consistent display.The friendGames array is created from the gameMap values without any sorting, which may lead to inconsistent ordering in the UI.
-const friendGames = Array.from(gameMap.values()); +const friendGames = Array.from(gameMap.values()).sort((a, b) => { + // Sort by name as the primary key + return a.name.localeCompare(b.name); +});
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (11)
apps/nextjs/next.config.js(1 hunks)apps/nextjs/src/app/dashboard/friends/[id]/shared/_components/friend-shared-items.tsx(1 hunks)apps/nextjs/src/app/dashboard/friends/[id]/stats/_components/friend-stat-page.tsx(1 hunks)apps/nextjs/src/app/dashboard/friends/[id]/stats/_components/game-details.tsx(1 hunks)apps/nextjs/src/app/dashboard/friends/[id]/stats/page.tsx(1 hunks)apps/nextjs/src/app/dashboard/friends/_components/friend-request-card.tsx(2 hunks)apps/nextjs/src/app/dashboard/games/[id]/[matchId]/summary/_components/match-summary.tsx(1 hunks)apps/nextjs/src/app/dashboard/players/[id]/stats/_components/GameDetailsTable.tsx(1 hunks)packages/api/src/routers/friends.ts(10 hunks)packages/api/src/utils/clerk.ts(1 hunks)packages/api/src/utils/game.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/api/src/utils/clerk.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/nextjs/src/app/dashboard/friends/[id]/stats/page.tsx
- apps/nextjs/src/app/dashboard/friends/[id]/shared/_components/friend-shared-items.tsx
🧰 Additional context used
🧬 Code Graph Analysis (2)
apps/nextjs/src/app/dashboard/friends/[id]/stats/_components/friend-stat-page.tsx (7)
packages/ui/src/avatar.tsx (3)
Avatar(50-50)AvatarImage(50-50)AvatarFallback(50-50)packages/shared/src/utils.ts (1)
formatDuration(14-19)packages/ui/src/badge.tsx (1)
Badge(37-37)apps/expo/src/lib/icons/Dices.tsx (1)
Dices(6-6)packages/ui/src/calendar.tsx (1)
Calendar(68-68)packages/ui/src/scroll-area.tsx (2)
ScrollArea(48-48)ScrollBar(48-48)apps/nextjs/src/app/dashboard/friends/[id]/stats/_components/game-details.tsx (1)
GameDetails(25-147)
packages/api/src/routers/friends.ts (4)
packages/api/src/utils/clerk.ts (1)
getFullName(3-17)packages/api/src/trpc.ts (1)
protectedUserProcedure(182-182)packages/api/src/utils/sharing.ts (1)
collectShares(164-293)packages/api/src/utils/game.ts (1)
mapMatches(3-274)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: lint
🔇 Additional comments (17)
apps/nextjs/src/app/dashboard/games/[id]/[matchId]/summary/_components/match-summary.tsx (1)
109-109: Good improvement to React key uniqueness.Using a composite key that combines
match.idandmatch.typeis a better approach to ensure uniqueness when different types of matches might share the same ID. This follows React best practices for stable keys in lists.apps/nextjs/src/app/dashboard/players/[id]/stats/_components/GameDetailsTable.tsx (1)
100-100: Good improvement to React key uniqueness.Using a composite key that combines
game.idandgame.typeensures uniqueness when different types of games might share the same ID. This follows React best practices and maintains consistency with similar changes in other components.apps/nextjs/src/app/dashboard/friends/[id]/stats/_components/game-details.tsx (1)
1-148: Well-implemented component with good performance considerations.The
GameDetailscomponent is well structured with proper sorting functionality, responsive design, and performance optimizations through memoization. The use of TypeScript for type safety is solid, and the component integrates well with the friend statistics feature described in the PR objectives.apps/nextjs/src/app/dashboard/friends/_components/friend-request-card.tsx (5)
185-189: The cancel button loading state is now properly implemented.The code correctly checks for the specific "cancel" loading state, addressing the issue noted in previous reviews.
80-102: Good implementation of the cancel friend request mutation.The new mutation for canceling friend requests follows the same pattern as the other mutations, ensuring consistent error handling and query invalidation.
126-193: Well-structured UI with clear conditional rendering.The component now elegantly handles both received and sent requests with appropriate UI for each case, using the
typeprop to conditionally render either Accept/Reject buttons or a Cancel button.
129-139: Nice avatar implementation with fallback.The use of Avatar with AvatarFallback provides a good user experience when image URLs are unavailable, gracefully falling back to user initials.
143-146: Good date formatting with helpful visual cue.The date display includes a clock icon and uses the formatDate utility, making it easy for users to see when the request was created.
apps/nextjs/src/app/dashboard/friends/[id]/stats/_components/friend-stat-page.tsx (4)
42-45: Good error handling with redirect.Checking for the existence of the linked player and redirecting when absent prevents rendering errors and provides a better user experience.
147-205: Great implementation of conditional badge styling.The badges for match status (Won/Lost/In Progress) use different styling to visually indicate the status, enhancing usability.
256-323: Excellent implementation of the horizontal scrollable match list.Using ScrollArea with ScrollBar for the match list provides a clean and mobile-friendly way to browse through matches.
50-115: Well-structured summary card with comprehensive statistics.The summary card presents key player statistics in a clean, organized layout that works well on both mobile and desktop views.
packages/api/src/utils/game.ts (1)
206-208: Good use of Set to prevent duplicate matches.Using a Set with unique keys combining match ID and type is an effective way to prevent duplicate matches in the aggregated results.
packages/api/src/routers/friends.ts (4)
9-11: Good use of external utilities.Importing and using shared utilities like
getFullName,mapMatches, andcollectSharesimproves code reusability and maintainability.
122-124: Efficiently fetching Clerk user data in batches.Using
getUserListwith arrays of user IDs optimizes API calls to Clerk, resolving the previous N+1 query issue.Also applies to: 167-169, 210-212
828-876: Well-implemented friend settings mutation.The
updateFriendSettingsmutation properly handles both creating new settings and updating existing ones, with appropriate error handling.
714-778: Comprehensive statistics calculation for friend player data.The code efficiently aggregates match data, calculates win rates, and identifies unique opponents, providing rich statistics for the UI.
apps/nextjs/src/app/dashboard/friends/[id]/stats/_components/friend-stat-page.tsx
Outdated
Show resolved
Hide resolved
apps/nextjs/src/app/dashboard/friends/[id]/stats/_components/friend-stat-page.tsx
Outdated
Show resolved
Hide resolved
… of requesteeId - Modify the friends router to accept a username as input for sending friend requests, enhancing user experience by allowing searches by username. - Implement error handling for cases where the user is not found in Clerk or the database, ensuring robust feedback for the user. - Streamline the logic for inserting friend requests, improving clarity and maintainability of the codebase.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (6)
packages/api/src/routers/friends.ts (6)
28-28:⚠️ Potential issue
clerkClientis an object, not a function – remove the call operator
clerkClientis exported as an already-initialized SDK object. Calling it (await clerkClient()) throwsTypeError: clerkClient is not a functionat runtime.-const client = await clerkClient(); +const client = clerkClient;
136-136:⚠️ Potential issue
clerkClientis an object, not a function – remove the call operatorSimilar to the issue in
sendFriendRequest, usingclerkClientas a function will cause runtime errors.-const client = await clerkClient(); +const client = clerkClient;
181-181:⚠️ Potential issue
clerkClientis an object, not a function – remove the call operatorSame
clerkClientfunction call issue that needs to be fixed.-const client = await clerkClient(); +const client = clerkClient;
224-224:⚠️ Potential issue
clerkClientis an object, not a function – remove the call operatorSame
clerkClientfunction call issue that needs to be fixed.-const client = await clerkClient(); +const client = clerkClient;
684-684:⚠️ Potential issue
clerkClientis an object, not a function – remove the call operatorSame
clerkClientfunction call issue that needs to be fixed.-const client = await clerkClient(); +const client = clerkClient;
825-825:⚠️ Potential issue
clerkClientis an object, not a function – remove the call operatorSame
clerkClientfunction call issue that needs to be fixed.-const client = await clerkClient(); +const client = clerkClient;
🧹 Nitpick comments (2)
packages/api/src/routers/friends.ts (2)
709-709: Remove console.log statement before committingDebug logging should be removed before production code is committed.
-console.log(returnedFriend);
166-166: Consider breaking down complex queries into smaller, more manageable functionsThe query for
getFriendhas very deep nesting with multiple levels of relations. Consider breaking it into smaller queries or reusable query builders to improve readability and maintainability.Also applies to: 448-677
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
packages/api/src/routers/friends.ts(10 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/api/src/routers/friends.ts (4)
packages/api/src/trpc.ts (2)
createTRPCRouter(85-85)protectedUserProcedure(182-182)packages/api/src/utils/clerk.ts (1)
getFullName(3-17)packages/api/src/utils/sharing.ts (1)
collectShares(164-293)packages/api/src/utils/game.ts (1)
mapMatches(3-274)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: typecheck
- GitHub Check: lint
🔇 Additional comments (6)
packages/api/src/routers/friends.ts (6)
161-161: Use the importedgetFullNameutility function from../utils/clerkYou're correctly importing the
getFullNameutility function at line 9, which is good. This addresses a previous code duplication issue.
697-708: Good usage of thecollectSharesutility to aggregate share dataThe implementation correctly aggregates shared entities for both directions of sharing (from user to friend and from friend to user), making this data easily accessible in the frontend.
731-794: Good implementation of match statistics aggregationThe code effectively:
- Maps matches from both original and shared sources
- Aggregates game statistics including wins, plays, and win rates
- Collects unique opponents
- Calculates overall stats like total wins and play duration
This provides comprehensive statistics for the friend profile UI.
868-891: Well-implemented upsert logic for friend settingsThe mutation correctly handles both insert and update cases for friend settings, first checking if settings exist and then either creating or updating them accordingly.
138-140: Excellent use of batch fetching for Clerk usersPrevious code likely suffered from N+1 query issues when fetching user data. This implementation correctly batches requests for Clerk users, which is more efficient and puts less strain on rate limits.
Also applies to: 183-185, 226-228
844-867: Well-structured schema for friend settingsThe input validation schema for friend settings is comprehensive and type-safe. It covers all the necessary permissions and preferences for sharing different types of content between friends.
…orting - Update the friends router to include a new `friendPlayer` field, improving the retrieval of linked player information for friends. - Introduce a `linkedPlayerFound` boolean to indicate the presence of a linked player, enhancing data clarity. - Sort friend games by name for better organization and user experience during game retrieval.
… date formatting - Introduce a new FormattedDate component to standardize date display across various components, enhancing code reusability and maintainability. - Replace direct date formatting logic with the FormattedDate component in multiple files, improving consistency in date presentation. - Update imports and refactor existing components to utilize the new FormattedDate component, streamlining the codebase.
- Update the `mapSharedMatchPlayers` function to improve player mapping logic by utilizing a Map for unique playerId handling, enhancing performance and clarity. - Streamline the return process by consolidating the mapping and filtering steps, resulting in cleaner and more maintainable code.
…uests - Introduce the AddFriendDialog component to allow users to send friend requests via email or username, enhancing user interaction on the FriendsPage. - Update the FriendsPage to include the AddFriendDialog, improving the overall functionality of the friends dashboard. - Refactor the friends router to handle friend requests based on email or username, ensuring robust error handling and user feedback. - Enhance the FriendRequestCard component to invalidate queries related to sent friend requests, improving data consistency.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (6)
packages/api/src/routers/friends.ts (6)
184-185:⚠️ Potential issueSame error:
clerkClientis an object, not a functionThis is the same issue as above -
clerkClientshould not be called with parentheses.-const client = await clerkClient(); +const client = clerkClient;
229-230:⚠️ Potential issueSame error:
clerkClientis an object, not a functionThis is the same issue as above -
clerkClientshould not be called with parentheses.-const client = await clerkClient(); +const client = clerkClient;
273-274:⚠️ Potential issueSame error:
clerkClientis an object, not a functionThis is the same issue as above -
clerkClientshould not be called with parentheses.-const client = await clerkClient(); +const client = clerkClient;
735-736:⚠️ Potential issueSame error:
clerkClientis an object, not a functionThis is the same issue as above -
clerkClientshould not be called with parentheses.-const client = await clerkClient(); +const client = clerkClient;
877-878:⚠️ Potential issueSame error:
clerkClientis an object, not a functionThis is the same issue as above -
clerkClientshould not be called with parentheses.-const client = await clerkClient(); +const client = clerkClient;
44-45:⚠️ Potential issue
clerkClientis an object, not a function – remove the call operator
clerkClientis exported as an already-initialized SDK object from Clerk.
Calling it (await clerkClient()) will throwTypeError: clerkClient is not a functionat runtime.-const client = await clerkClient(); +const client = clerkClient;
🧹 Nitpick comments (1)
apps/nextjs/src/app/dashboard/friends/_components/add-friend-dialog.tsx (1)
128-142: Consider consolidating the conditional mutation callThe
handleSubmitfunction has separate but nearly identical mutation calls for email and username types.You could simplify this by using the spread operator:
function handleSubmit(value: string, type: "email" | "username") { setIsSubmitting(true); - if (type === "email") { - sendFriendRequestMutation.mutate({ - type, - email: value, - }); - } else { - sendFriendRequestMutation.mutate({ - type, - username: value, - }); - } + sendFriendRequestMutation.mutate({ + type, + ...(type === "email" ? { email: value } : { username: value }), + }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (4)
apps/nextjs/src/app/dashboard/friends/_components/add-friend-dialog.tsx(1 hunks)apps/nextjs/src/app/dashboard/friends/_components/friend-request-card.tsx(4 hunks)apps/nextjs/src/app/dashboard/friends/page.tsx(2 hunks)packages/api/src/routers/friends.ts(10 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/nextjs/src/app/dashboard/friends/page.tsx
🧰 Additional context used
🧬 Code Graph Analysis (1)
apps/nextjs/src/app/dashboard/friends/_components/friend-request-card.tsx (3)
apps/expo/src/utils/api.tsx (1)
RouterOutputs(57-57)packages/ui/src/hooks/use-toast.ts (2)
useToast(192-192)toast(192-192)packages/ui/src/avatar.tsx (3)
Avatar(50-50)AvatarImage(50-50)AvatarFallback(50-50)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: typecheck
- GitHub Check: lint
🔇 Additional comments (12)
packages/api/src/routers/friends.ts (4)
189-197: The type used for mapping includes fields from implementationThe mapped type includes
createdAtwhich is used in the implementation, but was missing in a previous version according to past review comments. This has been fixed by including it in the type.
747-759: Good implementation of shared content aggregationThe code effectively collects shared content in both directions (user to friend and friend to user) using the utility function
collectShares. This creates a clean separation of concerns and good data organization.
761-776: Clean early return pattern for friends without linked playersThe code properly handles the case where no linked player is found by returning early with the appropriate data structure. This is a good pattern that avoids unnecessary code nesting.
896-943: Well-structured friend settings mutation with proper validationThe
updateFriendSettingsmutation is well-implemented with:
- Typed input validation using Zod
- Proper database querying to check for existing settings
- Conditional logic to either insert or update settings
- Error handling for non-existent friends
apps/nextjs/src/app/dashboard/friends/_components/add-friend-dialog.tsx (3)
36-46: Clear schema validation for user inputsThe email and username schemas provide clear validation rules with descriptive error messages, which will help users understand input requirements.
51-68: Good dialog state management patternExplicitly managing the dialog open state with
useStateprovides better control over the dialog lifecycle and makes the component more reusable.
152-261: Well-structured tabbed interface with coordinated form stateThe implementation of the tabbed interface with separate form validation but unified submission handling is clean and maintainable. The UI provides clear user feedback during the submission process.
apps/nextjs/src/app/dashboard/friends/_components/friend-request-card.tsx (5)
16-19: Improved props interface with type safetyUsing the RouterOutputs type for the friendRequest prop ensures type safety and consistent data structure throughout the application.
29-31: Expanded loading state to include cancel actionThe loading state now properly includes "cancel" as a possible value, which improves the user experience by providing appropriate feedback during all async operations.
86-111: Well-implemented mutation for canceling friend requestsThe
cancelFriendRequestMutationfollows the same pattern as the other mutations, ensuring consistent behavior and error handling.
135-203: Clean, responsive card layout with appropriate action buttonsThe card layout with avatar, name, and formatted date provides a good user experience. The conditional rendering of buttons based on request type is well-implemented.
194-198: Correctly implemented loading state check for cancel buttonThe cancel button correctly checks for the specific "cancel" loading state, which was an issue mentioned in past reviews that has been fixed.
|
Caution An unexpected error occurred while opening a pull request: Not Found - https://docs.github.com/rest/git/refs#get-a-reference |
✅ Checklist
Changelog
friendSettingandfriendPlayerschemas to support per-friend sharing preferencesfriendSettingschema to allow shared games, matches, players, and locations by defaultSummary by CodeRabbit
New Features
Enhancements
isUserflag for consistency.Bug Fixes
isUserflags.Database & Backend
friend_settingtable to store friend-specific sharing preferences.userIdwithisUserboolean and optionalfriendId.Chores